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

Add ListRepositoryTagsByReference rpc #2274

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

doriable
Copy link
Member

This adds a new RPC for ListRepositoryTagsForReference.
There is an argument to simply add to the old RPC, but given the way
our APIs have been structured, I think adding a new RPC to address this
need is better than simply adding to the old RPC. Even though it would still
be backwards compatible, the new RPC provides greater clarity.

Copy link
Member

@unmultimedio unmultimedio left a comment

Choose a reason for hiding this comment

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

Not 100% sure if a new RPC is better than just adding the missing reference param to the existing RPC. We'd need to know which places are using it, if any, and in the backend probably make it optional, so if it comes empty we can keep existing behavior?

Anyway, not blocking, just a thought.

Comment on lines 43 to 45
rpc ListRepositoryTags(ListRepositoryTagsRequest) returns (ListRepositoryTagsResponse) {
option idempotency_level = NO_SIDE_EFFECTS;
}
Copy link
Member

Choose a reason for hiding this comment

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

Wow I hadn't realized this one returns all tags in the repo and not from a single reference.

@doriable
Copy link
Member Author

Not 100% sure if a new RPC is better than just adding the missing reference param to the existing RPC. We'd need to know which places are using it, if any, and in the backend probably make it optional, so if it comes empty we can keep existing behavior?

Yeah, definitely. I think after the first run of sync, I'd like to document some of the API-related things, including this, and generally, should we be using the labels API, etc.
Appreciate this being not blocking :) Am definitely (attempting) to keep a log of all the things that are along these lines.

@doriable doriable merged commit 28e5bce into main Jul 10, 2023
9 checks passed
@doriable doriable deleted the BSR-2108-list-repository-tags-by-reference branch July 10, 2023 15:41
Copy link
Contributor

@seankimdev seankimdev left a comment

Choose a reason for hiding this comment

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

Sorry about the late review. Looks good to me. Thanks Doria

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.

3 participants