Conversation
mislav
left a comment
There was a problem hiding this comment.
Thanks for addressing this!
pkg/cmd/pr/merge/merge.go
Outdated
| switch status { | ||
| case "BLOCKED": | ||
| return !admin | ||
| case "BEHIND": |
There was a problem hiding this comment.
Does BEHIND allow the admin override? I've never tested this in the web UI.
There was a problem hiding this comment.
Oh that was just my assumption to what you wrote in #1198 (comment).
Probably wrong then.
There was a problem hiding this comment.
According to https://stackoverflow.com/questions/34463895/git-this-branch-is-behind-after-pull-request BEHIND seems to be rather informational only
There was a problem hiding this comment.
Not too sure how to force the BEHIND state yet (for a test).
pkg/cmd/pr/merge/merge.go
Outdated
| isPRAlreadyMerged := pr.State == "MERGED" | ||
| if blocked := blockedReason(pr.MergeStateStatus); !opts.AutoMergeEnable && !isPRAlreadyMerged && blocked != "" { | ||
| fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is not mergeable: %s.\n", cs.FailureIcon(), pr.Number, blocked) | ||
| if !opts.AutoMergeEnable && !isPRAlreadyMerged && isBlocked(pr.MergeStateStatus, opts.Admin) { |
There was a problem hiding this comment.
Instead of a new function isBlocked which mostly duplicates blockedReason, could blockedReason simply accept an extra Admin argument and adjust logic accordingly?
There was a problem hiding this comment.
Agree on that, thought about a double return as well after i commited it.
There was a problem hiding this comment.
Changed blockedReason accordingly but i am not to sure about the additional bool return (keeps it concise though).
Open for ideas.
pkg/cmd/pr/merge/merge.go
Outdated
| if !opts.AutoMergeEnable && !isPRAlreadyMerged && isBlocked(pr.MergeStateStatus, opts.Admin) { | ||
| fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is not mergeable: %s.\n", cs.FailureIcon(), pr.Number, blockedReason(pr.MergeStateStatus)) | ||
| fmt.Fprintf(opts.IO.ErrOut, "To have the pull request merged after all the requirements have been met, add the `--auto` flag.\n") | ||
| if !opts.Admin && !isBlocked(pr.MergeStateStatus, true) { |
There was a problem hiding this comment.
Instead of !isBlocked(pr.MergeStateStatus, true), which might be hard to parse for a human, what do you think about a new function called allowsAdminOverride(pr.MergeStateStatus)? I think something like that could communicate intent better
pkg/cmd/pr/merge/merge.go
Outdated
| BodySet bool | ||
| Editor editor | ||
|
|
||
| Admin bool |
There was a problem hiding this comment.
Could this be called UseAdmin so it reads better? The cli flag can stay the same for simplicity 👍
a020b8a to
baa18c1
Compare
Fixes #1198