Skip to content
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

Makes gcp attributes accessible #114

Merged
merged 5 commits into from
Jul 6, 2017
Merged

Makes gcp attributes accessible #114

merged 5 commits into from
Jul 6, 2017

Conversation

wezzle
Copy link
Contributor

@wezzle wezzle commented Jul 4, 2017

Just like my last pull request adding a function to Subscriber, this pull request adds a function to SubMessage making the underlying attributes sent with GCP accessible.

Wesley Bos added 4 commits June 30, 2017 10:16
- Makes gcp.subscriber public
- Makes gcp.subscribeImpl.sub public
- Applies case changes to tests
- Makes subMessage public
- Makes messageImpl's msg public
- Channel runts SubMessage instead of interface
@coveralls
Copy link

Coverage Status

Coverage remained the same at 45.194% when pulling 61c2a7c on wezzle:gcp-attributes into 4956979 on NYTimes:master.

@jprobinson
Copy link
Contributor

Unfortunately, this change breaks the interface contract on gizmo/pubsub.Subscriber:

	*Subscriber does not implement "github.com/NYTimes/gizmo/pubsub".Subscriber (wrong type for Start method)
		have Start() <-chan *SubMessage
		want Start() <-chan "github.com/NYTimes/gizmo/pubsub".SubscriberMessage

Is there any way you can encapsulate the information you need inside the message payload? Perhaps an 'envelope'-like struct that holds metadata and the payload?

@wezzle
Copy link
Contributor Author

wezzle commented Jul 6, 2017

Sorry, totally missed that. I'll take another stab at it.

- Adds test for pubsub.Subscriber interface
- Adds Attributes to SubMessage
- Fills Attributes on Receive if msg is of type messageImpl
- Allows for type conversion by external code on SubscriberMessage
@wezzle
Copy link
Contributor Author

wezzle commented Jul 6, 2017

Ok so i've changed back the interface part to pubsub.Subscriber. The public gcp.SubMessage now has Attributes that is filled on Receive if msg is of type messageImpl to make sure code does not break on type conversion during tests where the msg is of type testMessage.

External code can now use msg.(*gcp.SubMessage).Attributes to access the original GCP Attributes sent with the message itself.

Also i've added a test to prevent future errors with the pubsup.Subscriber interface.

Please let me know if you see anything wrong with this, or any improvements.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 43.967% when pulling 5d4750c on wezzle:gcp-attributes into 4956979 on NYTimes:master.

@jprobinson
Copy link
Contributor

Ok, I think that's an acceptable workaround for folks who really need the attributes.

Looks like we have some kind of merge conflict on this now. If you can clean that up, I'll pull this in.

@wezzle
Copy link
Contributor Author

wezzle commented Jul 6, 2017

How can I see what the conflict is? The message under this pull-request shows there are no conflicts with the base branch?

@jprobinson
Copy link
Contributor

Ah, it just wouldn't let me 'rebase and merge'. Squashing and merging...

@jprobinson jprobinson merged commit 8b0f780 into nytimes:master Jul 6, 2017
@jprobinson
Copy link
Contributor

🌮 thanks again!

@wezzle wezzle deleted the gcp-attributes branch July 6, 2017 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants