surface merge conflicts in pr status #5999

mntlty · 2022-07-25T23:24:49Z

closes #872

The field mergeStateStatus does not indicate if a merge is blocked due to a merge conflict. As an example, here is the response on a PR that has conflicts

{
	"data": {
		"repository": {
			"pullRequest": {
				"id": "PR_XXX",
				"mergeStateStatus": "DIRTY",
				"mergeable": "CONFLICTING"
			}
		}
	}
}

It is also in a preview and subject to change. The field mergeable indicates if there a conflict with the pull request, and is already part of the api.PullRequest struct.

I'm not sure if the color, icon and text are the best to display this information, open to feedback!

Update:

Following this comment from @samcoe I've added a flag called show-conflicts with the intent that the additional field mergeable can be easily rolled into the default fields.

The flag will be effectively ignored if the user also passes --json, as that argument list will take precedence over the default fields, which is the current behavior without the flag.

mntlty · 2022-07-25T23:25:49Z

tested using the -R flag in a different repo, and with the current branch

mislav · 2022-07-26T09:17:24Z

pkg/cmd/pr/status/http.go

@@ -189,7 +189,7 @@ func pullRequestStatus(httpClient *http.Client, repo ghrepo.Interface, options r
 func pullRequestFragment(httpClient *http.Client, hostname string) (string, error) {
 	fields := []string{
 		"number", "title", "state", "url", "isDraft", "isCrossRepository",
-		"headRefName", "headRepositoryOwner", "mergeStateStatus",
+		"headRefName", "headRepositoryOwner", "mergeable", "mergeStateStatus",


Requesting the mergeable field kicks off a potentially expensive (and sometimes async) job of calculating mergeability of a PR. Requesting this field on all records when fetching PRs in bulk might have negative consequences. Do you have thoughts about that?

@mislav I'll check with my team about this concern and see if any of the experts have concerns or can suggest an alternative.

we're making internal changes that will improve the cost of this operation. let's revisit at the end of august and see where things are with that work

vilmibm · 2022-08-22T16:31:51Z

Marking blocked until end of august

samcoe · 2022-09-28T05:58:46Z

@mntlty We discussed this offline as a team, with the still unknowns around the performance impact of the mergeable field on GHES and no easy way from the code to detect if a version of GHES has the performance changes implemented, we would like to see this PR changed to include a flag so users can opt into displaying merge conflicts rather than do it by default.

In the future we can start to include it by default and deprecate the flag but feel like this is the best path forward so that all our users have a good experience with pr status command.

mntlty · 2022-09-28T06:00:49Z

@mntlty We discussed this offline as a team, with the still unknowns around the performance impact of the mergeable field on GHES and no easy way from the code to detect if a version of GHES has the performance changes implemented, we would like to see this PR changed to include a flag so users can opt into displaying merge conflicts rather than do it by default.

In the future we can start to include it by default and deprecate the flag but feel like this is the best path forward so that all our users have a good experience with pr status command.

@samcoe that makes sense! Do you have a suggestion for what the flag should be called?

samcoe · 2022-09-28T06:17:43Z

I don't think we need to overthink it. Calling it something like show-conflicts seem reasonable to me. From the docs it looks like the mergable field only shows if the pull request cannot be merged due to merge conflicts so having "conflicts" in the name seems like the right move.

mntlty · 2022-09-29T00:07:15Z

pkg/cmd/pr/status/http.go

@@ -186,12 +187,16 @@ func pullRequestStatus(httpClient *http.Client, repo ghrepo.Interface, options r
 	return &payload, nil
 }

-func pullRequestFragment(httpClient *http.Client, hostname string) (string, error) {
+func pullRequestFragment(hostname string, showConflicts bool) (string, error) {


this is private function made sense to change the signature to support this change

mntlty · 2022-09-29T00:07:22Z

pkg/cmd/pr/status/http.go

@@ -186,12 +187,16 @@ func pullRequestStatus(httpClient *http.Client, repo ghrepo.Interface, options r
 	return &payload, nil
 }

-func pullRequestFragment(httpClient *http.Client, hostname string) (string, error) {


httpClient isn't used in this function

mntlty · 2022-09-29T00:41:18Z

@samcoe I've tested this with the new flag and without, with the json flag and without. I think this is ready for review 🙏

mntlty · 2022-09-29T05:39:52Z

pkg/cmd/pr/status/status.go

@@ -57,6 +58,7 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co
 		},
 	}

+	cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Show merge conflicts on pull requests")


does it make sense for this to have a short alias, maybe -c?

I think that makes sense to add the short alias since --show-conflicts is a bit verbose.

Suggested change

cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Show merge conflicts on pull requests")

cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Display pull request merge conflict status")

I am not sure my wording is the best but I wanted the long description to clarify that this flag will show an indicator if the pull request has a merge conflict and not have confusion where people expect the whole merge conflict to be displayed.

samcoe

@mntlty Thanks for making the requested changed. Code looks good to me. Left a couple questions regarding UX for follow up. Once those are answered I think this is good to go.

samcoe · 2022-09-29T06:55:22Z

pkg/cmd/pr/status/status.go

@@ -57,6 +58,7 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co
 		},
 	}

+	cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Show merge conflicts on pull requests")


I think that makes sense to add the short alias since --show-conflicts is a bit verbose.

Suggested change

cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Show merge conflicts on pull requests")

cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Display pull request merge conflict status")

I am not sure my wording is the best but I wanted the long description to clarify that this flag will show an indicator if the pull request has a merge conflict and not have confusion where people expect the whole merge conflict to be displayed.

samcoe · 2022-09-29T07:05:04Z

pkg/cmd/pr/status/status.go

@@ -264,6 +268,10 @@ func printPrs(io *iostreams.IOStreams, totalCount int, prs ...api.PullRequest) {
 				fmt.Fprint(w, cs.Green(fmt.Sprintf("✓ %s Approved", s)))
 			}

+			if pr.Mergeable == "CONFLICTING" {
+				fmt.Fprintf(w, " %s", cs.Red("× Merge conflicts"))


Do you think we should display if there are no merge conflicts? I feel like all the other status indicators displayed have a success status.

I think having explicit colors and text for each of the possible states, mergeable, conflicting and unknown is an improvement 👍

mntlty · 2022-09-29T15:32:27Z

pkg/cmd/pr/status/status.go

+				fmt.Fprintf(w, " %s", cs.Green("✓ No merge conflicts"))
+			} else if pr.Mergeable == "CONFLICTING" {
+				fmt.Fprintf(w, " %s", cs.Red("× Merge conflicts"))
+			} else if pr.Mergeable == "UNKNOWN" {


this should be explicit rather than a default "else" or we will end up showing it on all PRs when the user doesn't provide a flag

samcoe

@mntlty This is great thanks for making the requested changes, I pushed a small polish commit, I think this is ready to ship.

mntlty · 2022-10-03T09:28:35Z

@mntlty This is great thanks for making the requested changes, I pushed a small polish commit, I think this is ready to ship.

@samcoe thanks for all the feedback!

mntlty requested a review from a team as a code owner July 25, 2022 23:24

mntlty requested review from samcoe and removed request for a team July 25, 2022 23:24

cliAutomation added the external pull request originating outside of the CLI core team label Jul 25, 2022

cliAutomation added this to Needs review 🤔 in The GitHub CLI Jul 25, 2022

samcoe self-assigned this Jul 26, 2022

mislav reviewed Jul 26, 2022

View reviewed changes

vilmibm added the blocked label Aug 22, 2022

samcoe removed the blocked label Sep 19, 2022

show conflicts in PR status

5da1f1a

mntlty force-pushed the mntlty/pr_status_conflicts branch from 882cda7 to 5da1f1a Compare September 28, 2022 23:44

mntlty commented Sep 29, 2022

View reviewed changes

samcoe reviewed Sep 29, 2022

View reviewed changes

mntlty added 2 commits September 29, 2022 00:12

add shorthand for flag

c754e7a

code review feedback

f30b56d

mntlty commented Sep 29, 2022

View reviewed changes

mntlty and others added 2 commits September 29, 2022 08:36

rename variable to match

981ef91

Small polish

d09effc

samcoe approved these changes Oct 3, 2022

View reviewed changes

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Oct 3, 2022

samcoe enabled auto-merge (squash) October 3, 2022 09:26

samcoe merged commit 577d422 into trunk Oct 3, 2022

samcoe deleted the mntlty/pr_status_conflicts branch October 3, 2022 09:32

The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Oct 3, 2022

github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

surface merge conflicts in pr status #5999

surface merge conflicts in pr status #5999

mntlty commented Jul 25, 2022 •

edited

mntlty commented Jul 25, 2022 •

edited

mislav Jul 26, 2022

mntlty Jul 26, 2022

mntlty Jul 26, 2022

vilmibm commented Aug 22, 2022

samcoe commented Sep 28, 2022

mntlty commented Sep 28, 2022

samcoe commented Sep 28, 2022

mntlty Sep 29, 2022

mntlty Sep 29, 2022

mntlty commented Sep 29, 2022

mntlty Sep 29, 2022

samcoe Sep 29, 2022

samcoe left a comment

samcoe Sep 29, 2022

samcoe Sep 29, 2022

mntlty Sep 29, 2022

mntlty Sep 29, 2022

samcoe left a comment

mntlty commented Oct 3, 2022

	cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Show merge conflicts on pull requests")
	cmd.Flags().BoolVar(&opts.ShowConflicts, "show-conflicts", false, "Display pull request merge conflict status")

surface merge conflicts in pr status #5999

surface merge conflicts in pr status #5999

Conversation

mntlty commented Jul 25, 2022 • edited

Update:

mntlty commented Jul 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilmibm commented Aug 22, 2022

samcoe commented Sep 28, 2022

mntlty commented Sep 28, 2022

samcoe commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mntlty commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samcoe left a comment

Choose a reason for hiding this comment

mntlty commented Oct 3, 2022

mntlty commented Jul 25, 2022 •

edited

mntlty commented Jul 25, 2022 •

edited