-
Notifications
You must be signed in to change notification settings - Fork 243
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 --git-metadata
flag to buf push
#2953
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I think we could benefit from some tests on the git logic (we could easily run git init
in a temp dir and test various scenarios).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the generic logic needs to be put into the appropriate pkg,
bufpkg, or
bufpackages, in this case mostly
git`
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a blocker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple stylistic things.
Co-authored-by: Saquib Mian <[email protected]>
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Co-authored-by: Philip K. Warren <[email protected]>
return nil, err | ||
} | ||
runner := command.NewRunner() | ||
uncommittedFiles, err := git.CheckForUncommittedGitChanges(ctx, runner, input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-metadata
flag tobuf push
:--label
flag(s),--source-control-url
, and--create-default-label
to the HEAD branch of the default Git remote--source-control-url
,--create-default-label
,--label
,--tag
,--branch
, or--draft
labels alongsideThis PR also changes the default visibility for
--create-visibility
to "private". This means that users are no longer required to specify the--create-visibility
flag 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-url
flags.