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

feat(pubsub): add list configs for topic & sub #4607

Merged
merged 10 commits into from
Sep 8, 2021

Conversation

hongalex
Copy link
Member

When calling client.Topics and client.Subscriptions, you get back an iterator that allows you to iterate over all resources. However, this gives you the actual Topic and Subscription struct that's defined by the library and does not list configs, which currently need to be retrieved from an additional config call. This adds NextConfig to both Topic/SubscriptionIterator so that the config can be viewed instead of Topic/Subscription. This is the desired behavior, and is used for SchemaConfig as well as topic/subscription configs in Pub/Sub Lite.

Thanks to @mgabeler-lee-6rs for suggesting this.

Closes #4559

@hongalex hongalex requested review from a team as code owners August 13, 2021 05:08
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 13, 2021
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Aug 13, 2021
pubsub/subscription.go Outdated Show resolved Hide resolved
@hongalex hongalex requested a review from noahdietz September 1, 2021 20:46
Copy link
Contributor

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but another pubsub reviewer might want to approve

@hongalex hongalex merged commit a6550c5 into googleapis:master Sep 8, 2021
@hongalex hongalex deleted the pubsub-add-list-cfg branch September 8, 2021 22:14
@mgabeler-lee-6rs
Copy link

@hongalex I just tried to pick this up in my app, and ... I realized I missed something pretty important in my original suggestion: the SubscriptionConfig and TopicConfig objects returned from these don't have any information about the identity of the returned objects, which makes them nigh useless 🤦‍♂️

What do you think about adding a name field and String() and ID() methods to TopicConfig and SubscriptionConfig to match Topic and Subscription?

@hongalex
Copy link
Member Author

hongalex commented Sep 15, 2021

Oof, yeah you're totally right. I think the original intent for Topic/SubscriptionConfig was purely for storing config (regardless of the actual topic/config it is associated with). I'll draft up a fix for this today. Thanks for reporting as always!

@mgabeler-lee-6rs
Copy link

@hongalex guessing this got lost in the shuffle (no hard feelings!). Want me to put an issue up for the followup fix?

@hongalex
Copy link
Member Author

hongalex commented Oct 6, 2021

@mgabeler-lee-6rs apologies, indeed I created a starting point but slipped through the radar. I just created #4952 to track this better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pubsub: list subscription configs, not just names
4 participants