Replace CreateTags with SetTags
#33
Conversation
| // - If a Commit is referenced, Tags are set for that Commit. | ||
| // - If a Tag is referenced, Tags are set for the Commit that this Tag references. | ||
| // - If a VCSCommit is referenced, Tags are set for the Commit that this VCSCommit references. | ||
| // - Is a Branch is referenced, Tags are set for the Commit that this Branch references. |
There was a problem hiding this comment.
| // - Is a Branch is referenced, Tags are set for the Commit that this Branch references. | |
| // - Is a Branch is referenced, Tags are set for the latest Commit on the Branch this references. |
| (buf.validate.field).required = true, | ||
| (buf.validate.field).string.max_len = 250 | ||
| ]; | ||
| // The id of the Commit associated with the Tag. |
There was a problem hiding this comment.
How can you create a commit without a commit_id to reference? Every Tag has a commit id.
There was a problem hiding this comment.
Because the commit would be resolved by the passed ResourceRef. This was motivated mainly by Buf Sync in which we have map[git.Hash][]tags.
| @@ -92,31 +91,36 @@ message ListTagsResponse { | |||
| repeated Tag tags = 2; | |||
| } | |||
|
|
|||
| message CreateTagsRequest { | |||
| // An individual request to create a Tag. | |||
| message SetTagsRequest { | |||
There was a problem hiding this comment.
Why not just UpdateTags, in the same style as other RPCs?
There was a problem hiding this comment.
Because semantically Update infers that the Tag existed before, I think? This RPC creates or moves the tag.
There was a problem hiding this comment.
I think the idea behind this was that we don't care if it exists or not, just put it here. Without this, the flow is
GetTags- If Tag doesn't exist, call
CreateTag; otherwise callUpdateTags.
I'm fine with either approach, especially because sync will already call GetTags (to only sync tags that have changed).
There was a problem hiding this comment.
Are there any real downsides to the non-atomic approach?
There was a problem hiding this comment.
If we have:
Createcreate a tag that previously didn't existCreateOrUpdatecreates any tags that previously didn't exist, updates any that previously existed
Then I'm happy. I think Create needs to stay because it's impossible to say "create this tag and I want to be sure I'm not moving the tag and potentially breaking someone".
There was a problem hiding this comment.
I'm cool either way. I'll close this one in favor of #32 then, and comment there.
|
Closing in favor of #32 to keep atomic version: |
An alternative to #32.
For tags most of the times we don't need two different RPCs, and for sync we always override/move the current tag position, or just create it if it doesn't exist.
In the case we ever need to create-only or move-only, the client can use the existing
GetTagRPC first.