Add --git-metadata flag to buf push
#2953
Conversation
| // with a git checkout. | ||
| return nil, fmt.Errorf("unable to check input %q, please ensure this is a Git repository checkout: %w", input, err) | ||
| } | ||
| if len(uncommittedFiles) > 0 { |
There was a problem hiding this comment.
This has always been a part of the product spec -- we do not allow users to push unchecked/uncommitted references. This makes sense, since we are tagging this with Git commit information through the VCS -- if there are uncommitted changes being pushed, that commit information would not make sense.
|
Resolved old conversations around URL parsing and erroring behaviours (based on today's conversation), refactored git commands for metadata to |
Co-authored-by: Saquib Mian <smian@buf.build>
| // If the remote hostname contains github (e.g. github.mycompany.com or github.com) or gitlab | ||
| // (e.g. gitlab.mycompany.com or gitlab.com) then it uses the route /commit for the git | ||
| // commit sha. | ||
| func getGitMetadataSourceControlURLUploadOption( |
There was a problem hiding this comment.
I think it would be good to move the building of source control URLs to the git package in order to keep the logic in here purely around upload. Perhaps a SourceControlURL(commitSHA string) on the git.Remote interface?
There was a problem hiding this comment.
Hmm, yeah, I went back and forth a little bit on where specifically this logic should live, since it's kind of "business-y" (rather than "generically git"), but also, the RemoteKind type already breaks that a little bit. So given that, it might make sense to put there.
It is a little strange to have this function take commitSHA, but I think that is reasonable. I'll make it clear that it just accepts the commitSHA as a string but doesn't do any validation against that string.
There was a problem hiding this comment.
Actually, in this case, we wouldn't need to expose RemoteKind at all. I think I'm going to unexport the enum and keep it since it is useful for keeping the parsing logic clean (and independent from the initial parsing of the URL).
Co-authored-by: Philip K. Warren <pkwarren@users.noreply.github.com>
Co-authored-by: Philip K. Warren <pkwarren@users.noreply.github.com>
| return nil, err | ||
| } | ||
| runner := command.NewRunner() | ||
| uncommittedFiles, err := git.CheckForUncommittedGitChanges(ctx, runner, input) |
There was a problem hiding this comment.
To what I can see, there is no verification here that input is in fact a directory, however it is expected by git.CheckForUncommitedGitChanges that input is a path to a local directory. This validation needs to be done, and should be obvious in the context of this function.
There was a problem hiding this comment.
Added validation here.
It is worth noting that git.CheckForUncommittedGitChanges would return an error that indicates if the provided input is not a valid directory and was captured in the error handling below, but it's safer to more explicit with the validation here.
| } | ||
| sourceControlURL := originRemote.SourceControlURL(currentGitCommit) | ||
| if sourceControlURL == "" { | ||
| return nil, appcmd.NewInvalidArgumentError("unable to determine source control URL for this repository; only GitHub/GitLab/BitBucket are supported") |
There was a problem hiding this comment.
Why does it matter? This is a severe limitation, and excludes any github enterprise customer. We should be able to parse a source control url for any http/https/ssh endpoint
There was a problem hiding this comment.
There are two potential questions here so will answer both.
Why is it an error if we can't infer a source control url?
If the user provided --git-metadata then they are expecting us to automatically set some values. If we can't set the values they are expecting, we should error. This means during setup, they will catch the problem early, and then later if, due to a code host migration, it breaks, they will know instantly.
Why are we limited to GitHub/Bitbucket/GitLab?
The specification for Remote.SourceControlURL() (which is in the interface documentation in this PR) is designed to work for enterprise customers as long as the name of their code host is in the remote url. If it isn't, we don't know what code host we are dealing with and therefore we don't know how to correctly construct the user facing url because the routes are different depending on the code host.
On GitHub and GitLab, routes look like https://<host><repo-path>/commit/<commit-sha>
and on Bitbucket, routes look like https://<host><repo-path>/commits/<commit-sha>
(note /commit/ vs /commits).
There was a problem hiding this comment.
The code quite literally checks for if the hostname includes github, gitlab, bitbucket, and just with a strings.Contains on the hostname. This both requires hostnames to include github, for example (this isn't a hard requirement), and it would also accept things like foogithubbar, which doesn't feel like what this filter wants to do in the first place.
This limitation isn't acceptable for our enterprise customers. Additionally, my impression was that "source control URL" wasn't mean to link to a specific point-in-time on the repository, it was just meant to link to the repository itself.
We can discuss deferring this until after release, but I consider this a bug - limiting acceptable hostnames to strings.Contains on one of github, gitlab, bitbucket isn't acceptable
| } | ||
| sourceControlURL := originRemote.SourceControlURL(currentGitCommit) | ||
| if sourceControlURL == "" { | ||
| return nil, appcmd.NewInvalidArgumentError("unable to determine source control URL for this repository; only GitHub/GitLab/BitBucket are supported") |
There was a problem hiding this comment.
There are two potential questions here so will answer both.
Why is it an error if we can't infer a source control url?
If the user provided --git-metadata then they are expecting us to automatically set some values. If we can't set the values they are expecting, we should error. This means during setup, they will catch the problem early, and then later if, due to a code host migration, it breaks, they will know instantly.
Why are we limited to GitHub/Bitbucket/GitLab?
The specification for Remote.SourceControlURL() (which is in the interface documentation in this PR) is designed to work for enterprise customers as long as the name of their code host is in the remote url. If it isn't, we don't know what code host we are dealing with and therefore we don't know how to correctly construct the user facing url because the routes are different depending on the code host.
On GitHub and GitLab, routes look like https://<host><repo-path>/commit/<commit-sha>
and on Bitbucket, routes look like https://<host><repo-path>/commits/<commit-sha>
(note /commit/ vs /commits).
This PR adds the
--git-metadataflag tobuf push:--labelflag(s),--source-control-url, and--create-default-labelto the HEAD branch of the default Git remote--source-control-url,--create-default-label,--label,--tag,--branch, or--draftlabels alongsideThis PR also changes the default visibility for
--create-visibilityto "private". This means that users are no longer required to specify the--create-visibilityflag when callingbuf push --create-- it will default to creating a private repository if one does not already exist.Also, this PR includes a changelog entry for all the changes above, as well as the
--label,--create-default-label, and--source-control-urlflags.