pr create - implement --push-to-base-repo flag
#8146
Conversation
6715a1a to
5d63839
Compare
|
Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message. |
pr create - implement --push-to-base-repo flag
| if headRepo == nil && isPushEnabled && opts.PushToBaseRepo { | ||
| pushableRepos, err := repoContext.HeadRepos() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, r := range pushableRepos { | ||
| if ghrepo.IsSame(baseRepo, r) { | ||
| headRepo = r | ||
| } | ||
| } | ||
|
|
||
| if headRepo == nil { | ||
| return nil, fmt.Errorf("the user might lack write access to the base repository") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Should this be refactored into the following section to avoid duplicating logic but leveraging (r *ResolvedRemotes) RemoteForRepo, which is doing similar logic already?
Lines 161 to 169 in 3443a75
There was a problem hiding this comment.
I'm probably missing something, but I don't see how we could incorporate the logic into the (r *ResolvedRemotes) RemoteForRepo method.
The proposed logic iterates over all pushable repos and determines if baseRepo can be pushed, just to fail early. Remotes, which RemoteForRepo handles, will only be used by the time we actually push on
cli/pkg/cmd/pr/create/create.go
Lines 741 to 743 in 363dacb
| fl.Bool("no-maintainer-edit", false, "Disable maintainer's ability to modify pull request") | ||
| fl.StringVar(&opts.RecoverFile, "recover", "", "Recover input from a failed run of create") | ||
| fl.StringVarP(&opts.Template, "template", "T", "", "Template `file` to use as starting body text") | ||
| fl.BoolVar(&opts.PushToBaseRepo, "push-to-base-repo", false, "Push the head branch to the base repository") |
There was a problem hiding this comment.
If we're going to expand flags to allow people to determine this from the command line, knowing there are more options that people want, then I imagine this might be more generic flag with different values for prompt, base, ...
There was a problem hiding this comment.
Interesting idea! I took a look at the code, but I couldn't find any other 'canonical' repository names except for base (which already has an implementation for that we're using).
For prompt, that's already the default behaviour if this flag isn't passed
cli/pkg/cmd/pr/create/create.go
Line 577 in 363dacb
Are you suggesting accepting any value here and validating it against the pushableRepos as seen on https://github.com/arturhoo/cli/compare/push-to-base-repo...arturhoo:cli:push-to-repo?expand=1 ?
|
Hey @andyfeller - thank you for taking the time to review my suggestion. I've replied to each suggestion individually and created a new issue #8152 - I would appreciate if you could take another look before I make further changes. Thanks! |
5d63839 to
db491aa
Compare
|
Closing, see #8152 (comment) for context. |
My workflow with private repositories usually consists of:
gh repo create --fillThe vast majority of times, the PR will be created in the base repository as defined by
cli/context/context.go
Line 60 in 3443a75
In other words, I'm pushing my local branch to the
originremote (the central, private repository) as part of the PR creation flow.This is currently done through an interactive prompt, where the base repository is presented as the first option - I've created the muscle memory of just pressing
<enter>right after runninggh pr createevery time.This PR introduces a new flag
--push-to-base-repothat skips this interactive prompt, when the user has write access to that repository.Fixes #8152
--
P.S.: this seems to have been discussed in #1718 - I think this approach might tackle at least the common workflow os folks who have clones a repository which they have write access to (and don't submit PRs through forks).