-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for publishSettings #1072
base: main
Are you sure you want to change the base?
add support for publishSettings #1072
Conversation
ProjectID: projectID, | ||
TopicID: topicID, | ||
SubscriptionID: subID, | ||
PublishSettings: &pubsub.PublishSettings{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change itself looks good to me, but I'm curious how this tests it. What in the test makes it fail and what would change if the PublishSettings were not specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not specified then default publish settings would be used as documented here https://github.com/googleapis/google-cloud-go/blob/265963bd5b91c257b3c3d3c1f52cdf2b5f4c9d1a/pubsub/topic.go#L76
With regards to what would make this test fail, it exists to more than anything to statically verify the setting is exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What verifies that the settings used came from what's on line 186 instead of the defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the change is to the interface there is a limit to how far we can verify this. that limit (without testing google's own pubsub client code) is that we can verify the Public variable is exposed in by the struct.
Is there some alternative you have in mind that I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see two other similar PRs to draw potential inspiration from:
- The original PR adding receiveSettings https://github.com/cloudevents/sdk-go/pull/396/files in which the default value is declared and then a simple check is performed in the test to see if the value is equal to the default.
- This PR exposing a custom host option https://github.com/cloudevents/sdk-go/pull/1070/files in which a setter function is added and unit tested.
There is an argument to be made that we should define our own default publish settings like receive although I don't really see how this would make the sdk more intuitive other then perhaps consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my underlying point here remains that the unit test for ReceiveSettings goes arguably less far then this publishSettings test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to make it so that w/o the parameter the operation works and then with your settings it fails? I could live with that much.
Or if that can't be done (e.g. because we need an instance of google pubsub and we don't have that), then perhaps try to get an error from the call to CreateTopic() that complains about a parameter in your settings. That'll prove your settings made it to the pubsub code.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JamesBLewis ping
2bf81bc
to
8213ea5
Compare
Signed-off-by: James Lewis <[email protected]>
8213ea5
to
0f2fcb1
Compare
Resolves #1026
Update Connection struct to allow for customising PublishSettings the same way ReceiveSettings can be customised.
previously raised as #1027