Add new branch RPCs and change existing RPCs#2283
Conversation
unmultimedio
left a comment
There was a problem hiding this comment.
More to the why of these RPC, why not adding a filter to the existing rpc ListRepositoryBranches, like bool default_branch to "list" only the default one. And when listing the non-default, it could be filtered out by the client using the is_main_branch from L29.
| option idempotency_level = NO_SIDE_EFFECTS; | ||
| } | ||
| // GetDefaultBranch returns the branch name that is mapped to the main/BSR_HEAD. | ||
| rpc GetDefaultBranch(GetDefaultBranchRequest) returns (GetDefaultBranchResponse) { |
There was a problem hiding this comment.
This RPC name conflicts with L28-29 in which we say it's not "default" but "main" :/
I like "default" better though.
There was a problem hiding this comment.
Ah that's true :< Hmm... yeah, I want to use the term default... tbh, I wish we could change the other RPC structure, lol... but it might be late for that. I think Default is a lot more explicit and clear for this use case though.
There was a problem hiding this comment.
Had an offline discussion, we also decided to go with Current, since this makes it clear it's the current/latest default branch, and does not reflect the history of default branches if it's ever been changed.
There was a problem hiding this comment.
Should the is_main_branch be removed? I think it may no longer be necessary with the addition of this new rpc. It also feels weird to have a field dedicated to indicate true only for one branch that is default(main).
There was a problem hiding this comment.
And when listing the non-default, it could be filtered out by the client using the is_main_branch from L29.
ah sorry I missed this part @unmultimedio wrote. I guess it would be necessary if we decide to go in that direction. Let's chat in the meeting.
| } | ||
| // ListGitCommitsNoMetadataForReference takes a string reference and returns all the git commit | ||
| // labels without metadata associated with the resolved reference commit. | ||
| rpc ListGitCommitsNoMetadataForReference(ListGitCommitsNoMetadataForReferenceRequest) returns (ListGitCommitsNoMetadataForReferenceResponse) { |
There was a problem hiding this comment.
I assume this RPC would be invoked when ListGitCommitMetadataForReference returns empty for a given reference?
There was a problem hiding this comment.
Yeah, so in the world where a user has buf push'd a BSR commit, that BSR commit reference will not have any git metadata, but may still have meaningful GIT_COMMIT_SHA labels that need to be displayed, so this RPC would be invoked for that circumstance.
| // Number of git commits with metadata associated with the BSR commit. | ||
| int64 git_commit_metadata_count = 13; | ||
| // Number of git commits without metadata associated with the BSR commit. | ||
| int64 git_commit_no_metadata_count = 14; |
There was a problem hiding this comment.
I'm confused with this one, so if a commit has 2 git commits associated with it, it still has 2 git commit labels. Unless I'm missing something, I think git_commit_no_metadata_count is not necessary.
There was a problem hiding this comment.
I guess it's for the circumstance where the BSR commit has no git commit metadata associated with it, but still has git commit labels on it -- we would want to count those.
There was a problem hiding this comment.
Not sure how this is going to play out. For a BSR commit that was previously push, with, say 3 tags:
buf push --tag foo --tag bar --tag someGitCommitSHA1
and then sync runs, and the content is the same, attaching two git metadatas to the same BSR commit:
someGitCommitSHA2
someGitCommitSHA3
I guess the commit object would have:
{
tag_count: 5, // foo, bar, and the 3 git shas
git_commit_metadata_count: 2, // someGitCommitSHA2 and someGitCommitSHA3
git_commit_no_metadata_count: 1 // someGitCommitSHA1
}
Am I right?
| // Number of git commits with metadata associated with the BSR commit. | ||
| int64 git_commit_metadata_count = 13; | ||
| // Number of git commits without metadata associated with the BSR commit. | ||
| int64 git_commit_no_metadata_count = 14; |
There was a problem hiding this comment.
Not sure how this is going to play out. For a BSR commit that was previously push, with, say 3 tags:
buf push --tag foo --tag bar --tag someGitCommitSHA1
and then sync runs, and the content is the same, attaching two git metadatas to the same BSR commit:
someGitCommitSHA2
someGitCommitSHA3
I guess the commit object would have:
{
tag_count: 5, // foo, bar, and the 3 git shas
git_commit_metadata_count: 2, // someGitCommitSHA2 and someGitCommitSHA3
git_commit_no_metadata_count: 1 // someGitCommitSHA1
}
Am I right?
|
@unmultimedio Yes, this is exactly correct. |
Adds a new RPC for branches,
GetDefaultBranch, which returns the current defaultbranch mapped to BSR main.
Adds three new fields to
RepositoryCommit:tag_count-- number of tags associated with the BSR commitgit_commit_metadata_count-- number of git commits with metadata associated with the BSR commitgit_commit_no_metadata_count-- number of git commit labels without metadata associated with the BSR commitAdds pagination of
ListGitMetadataForReference.Adds a new RPC for reference,
ListGitCommitsNoMetadataForReference, which returnsa paginated list of git commit labels that do not have git metadata.