GitHub Community
Improved pull request merge experience (public preview) feedback #143787
Replies: 996 comments 613 replies
-
|
Where is the "Merge without waiting for requirements to be met (bypass branch protections)" checkbox? Old New |
Beta Was this translation helpful? Give feedback.
-
|
Urrgss this doesn't look good at all |
Beta Was this translation helpful? Give feedback.
This comment was marked as spam.
This comment was marked as spam.
-
|
This is amazing |
Beta Was this translation helpful? Give feedback.
-
|
Isn't possible to have an option to always show the checks, with no inner scroll, like a normal page?? I don't understand why it needs to be hidden |
Beta Was this translation helpful? Give feedback.
-
|
Is it just me or do the check animations use a lot of gpu power? |
Beta Was this translation helpful? Give feedback.
This comment was marked as spam.
This comment was marked as spam.
-
|
Thanks for the feedback / write-up. Definitely agree. This check box appears for some users in some scenarios, but there's clearly a bug here where it's not appearing for everyone. We're investigating. |
Beta Was this translation helpful? Give feedback.
This comment was marked as off-topic.
This comment was marked as off-topic.
-
|
I like this feature so far, but the only transient bug I've seen is sometimes the checks show up fine while they are processing, but once all checks have passed, they all disappear. If you select the arrow to expand, it shows an empty list. I need to refresh the browser to see the passing checks. Again, just a small client-side issue that I'm seeing in Edge |
Beta Was this translation helpful? Give feedback.
-
|
I added this issue to the Known issues > Bugs section in the description above. Check there for updates, and to see other issues we're tracking. |
Beta Was this translation helpful? Give feedback.
-
for a second I thought you commented your IP |
Beta Was this translation helpful? Give feedback.
-
|
again non diversitalizing materials the only truely honest traite available to be earned. |
Beta Was this translation helpful? Give feedback.
-
|
just hoping for the best cus my time has been wasted looking for a way out of everything i put myself into just to find a better living |
Beta Was this translation helpful? Give feedback.
-
|
What might cause the checks to disappear after processing? |
Beta Was this translation helpful? Give feedback.
-
|
The "view command line instructions" can be handy. It used to be a clickable element that extends to show CLI commands. It is not anymore. Could be good to add back in? |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the feedback. This is coming! See the Known issues > Feature gaps list above. |
Beta Was this translation helpful? Give feedback.
-
|
The second command of Step 1 points to the wrong repository. It should point to the repo of the branch being merged, not the repo of the branch it's being merged into. The instructions of the "classic merge experience" are correct. |
Beta Was this translation helpful? Give feedback.
-
|
@mno-ly : surprisingly, yes. I see a fair share of samaritan contributors who are helping improve FOSS projects, even if they are young and making first steps with the language or git tooling, so helping them help us is good. A perhaps more "troublesome" category is contributors who are very much not young, and while they can share experience about the project or subject matter generally, and are comfortable about programming or documentation tools, learning new-fangled tools like git (and remembering it until the next time) is a practical problem, so cheat-sheets do help. May I live to their age and remain so eager and able to help! |
Beta Was this translation helpful? Give feedback.
This comment was marked as off-topic.
This comment was marked as off-topic.
-
|
The new look on that PR successfully merged message confused me.
Its appearance is quite similar to the message of you can click this button to delete the branch on the classic pane. This look makes me wonder if the auto delete branch feature is broken. |
Beta Was this translation helpful? Give feedback.
-
|
I think this feedback is deployed now. |
Beta Was this translation helpful? Give feedback.
-
|
@willsmythe It shows "...has been merged and deleted" (see above) to me but my branch is protected (and if it would have been deleted in fact, it would be a disaster because of deployment). |
Beta Was this translation helpful? Give feedback.
-
|
@richardhj +1 here too |
Beta Was this translation helpful? Give feedback.
-
|
Has there been any consideration to redesign or clarify the 'PR successfully merged and closed' message to avoid confusion with the auto-delete branch feature? |
Beta Was this translation helpful? Give feedback.
-
|
+1 here, very confusing |
Beta Was this translation helpful? Give feedback.
-
The "Delete branch" button CSS/style sometimes doesn't load. |
Beta Was this translation helpful? Give feedback.
-
|
Same happens occasionally in Firefox, Windows |
Beta Was this translation helpful? Give feedback.
-
|
Sorry for late reply, I'm seeing this on Arc browser. |
Beta Was this translation helpful? Give feedback.
-
|
This should be resolved now. If anyone is still experiencing this, let us know! |
Beta Was this translation helpful? Give feedback.
-
|
yes |
Beta Was this translation helpful? Give feedback.
-
|
Are there plans to implement a more robust solution to ensure consistent rendering of UI elements across all browsers? |
Beta Was this translation helpful? Give feedback.
-
|
Here is the new experience on a PR with an approval from a reviewer without write access, that has also passed all CI:
Displaying red around this feels a bit odd to me. My first thought when I see it is that CI has failed, changes have been requested, or maybe that there are required approvals/checks still being waited on? (None of these are the case in this repository, which is nixpkgs) In comparison, the classic merge experience has a green accent:
(I also noticed the new experience shows the approval of the reviewer w/o write access. This is an awesome addition for nixpkgs, so thanks!) The use of green lets a PR author or committer know at a glance that it should be good to merge. I'd much rather keep this behavior :) |
Beta Was this translation helpful? Give feedback.
-
|
Thank you, I came here for this! Red is a very bad choice for this considering CI did not fail and the merge is approved/conflict free. |
Beta Was this translation helpful? Give feedback.
-
|
As a side note, I really dislike that the passed checks are hidden, and that if I expand the successful checks, that it doesn't stay expanded on refresh or if I come back. Sometimes GitHub doesn't run the actions for whatever reason, or it's delayed, so seeing a blank/empty box here is an internal red flag for me, whereas if all is well I expect to see a line-up of green checkmarks |
Beta Was this translation helpful? Give feedback.
-
|
I also think that It shouldn't be red when there are no |
Beta Was this translation helpful? Give feedback.
-
|
+1 on changing the use of a red colour (and the big red cross icon) even if there are no issues. This is not very user-friendly and disguises real problems. Amber/orange makes more sense, and maybe a "paused" icon instead of a cross if the only thing needed is a review |
Beta Was this translation helpful? Give feedback.
-
|
+1 for not using red |
Beta Was this translation helpful? Give feedback.
-
|
Very good |
Beta Was this translation helpful? Give feedback.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
-
|
"Enable auto-merge" button is replaced with "Squash and auto-merge" which is broken: Console logs just displays 404 on POST to |
Beta Was this translation helpful? Give feedback.
-
|
The team is reviewing a fix for this. |
Beta Was this translation helpful? Give feedback.
-
|
A fix for this problem rolled out on Friday! 🎉 Let us know if you're still seeing this problem. |
Beta Was this translation helpful? Give feedback.
-
|
It's fixed now, thanks <3 |
Beta Was this translation helpful? Give feedback.
-
|
What steps were taken by the development team to address the problem, and how long did it take to deploy a fix? |
Beta Was this translation helpful? Give feedback.
-
|
Also, would be nice to bring back showing the other options (Merge commit, etc.) that may be disabled by the repository owner. That way you know what to ask for if your preference is missing |
Beta Was this translation helpful? Give feedback.
-
|
it's buggy and requires a page refresh before the merge button works even when all the checks are already passing, overall it feels less real time than before |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the feedback. Definitely a problem. We're investigating. |
Beta Was this translation helpful? Give feedback.
-
|
Are there any required status checks? Or just optional? Or a mix? |
Beta Was this translation helpful? Give feedback.
-
|
I have the same issue. We have several optional checks and one required. |
Beta Was this translation helpful? Give feedback.
-
|
A fix is underway for this. Should deploy by early next week. |
Beta Was this translation helpful? Give feedback.
-
|
Hey Will 👋. I ran into a simpler case where, if you merge the base branch and the checks pass, the "enable auto merge" button stays visible until you refresh. Seemed like an extension to this feedback 🙌. Great update btw! 🚀 |
Beta Was this translation helpful? Give feedback.
-
Sorry for the oversized screenshot! I noticed the text merge is not an allowed merge method in this repository in a repository I don't control - I'm assuming this isn't intended and so reporting it here. |
Beta Was this translation helpful? Give feedback.
-
|
The "Merge is not an allowed merge method in this repository." part of argoproj/argo-cd#21064:
definitely doesn't make sense. |
Beta Was this translation helpful? Give feedback.
-
|
@willsmythe If the intent here is that, say, only squash is allowed, then the |
Beta Was this translation helpful? Give feedback.
-
|
I just spun wheels trying to determine if I had set up my PR incorrectly, only to switch to classic merge experience and see that nothing seems to be wrong. This language makes no sense. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, this one is weird. I'm seeing something similar on a PR that I opened on a repo for which I do not have write access. And contrary to the message the branch in question does not contain a merge-commit.
|
Beta Was this translation helpful? Give feedback.
-
|
Ran into the same issue and spent way too much time thinking I did something wrong here when submitting a PR. The language is extremely confusing and misleading and should be adjusted. |
Beta Was this translation helpful? Give feedback.
-
|
I like that the list of checks is grouped collapsed now, though when expanding it, I'd want all of them to be opened by default, including successful checks. In the example below, it currently only expands skipped checks: I also feel like the red outline for awaiting reviews is a bit too aggressive, especially when coupled with removing so much green (no full checkmark as well as including skipped checks as gray in the outline). |
Beta Was this translation helpful? Give feedback.
-
|
Successful checks often includes diff, which important to check before approving, so keeping them easy accessible is very important. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, when a PR is fine, checks passed, just waiting review, it immediately threw me off to see it now in red. Red I usually associate with some action required on my part to resolve.
|
Beta Was this translation helpful? Give feedback.
-
|
My preference would be while checks are still running/pending/failed to either expand the checks group or have an account setting to automatically expand those groups by default. Passing checks defaulting to collapsed seems like a sensible choice as I'm not waiting on them and they would not require intervention. Therefore not seeing a list of checks screams things completed and are fine. |
Beta Was this translation helpful? Give feedback.
-
|
When UI elements are collapsed, it helps because there's often something below that I'd rather not scroll to. If not, then it's not helping me consolidate space for anything. Successful checks are already on the bottom, you're not saving me any effort by keeping them collapsed by default. But you are adding a click for when I do need them. |
Beta Was this translation helpful? Give feedback.
-
|
In this thread, it seems that we have two classes of users. Those who want to see things collapsed by default and those that don't. If you want it collapsed, you probably have a lot of jobs and just need to see what fails at a glance. Would it be so bad for this to be a remembered thing per user or repository? I collapsed successful jobs, so maybe that isn't useful to me or this repository. It seems like some elements of the new merge experience remember your choices, others do not. I'd just like to see this morph into something that is consistently useful for as many people as possible. For me, showing successful jobs is a problem. Showing skipped jobs, is a problem. At least one of the repositories I work with has a very big workflow and that's just not something I can parse. |
Beta Was this translation helpful? Give feedback.
-
THANK YOU 🙇🏻♂️ ❤️ 🥳 ! |
Beta Was this translation helpful? Give feedback.
-
|
I have a repository that has over 40 jobs that are commonly running and sometimes depending on what is changed, that can grow up to 76. Previously I had to scroll/hunt for the failing jobs A LOT. |
Beta Was this translation helpful? Give feedback.
-
|
We are glad to hear about this. GitHub developers have infinitely more than 76 on many of our repos (as do a ton of larger repositories using GitHub), so this was something we felt would really help the DX. Thank you for the feedback. |
Beta Was this translation helpful? Give feedback.
-
|
Bringing failing checks to the top is a huge win. Thanks for this 🙌 |
Beta Was this translation helpful? Give feedback.
-
|
I came to say the same, I routinely work on a repo that has 70+ checks, and collapsing the results, and moving the failing to the top makes this much nicer :) |
Beta Was this translation helpful? Give feedback.
-
|
Is there any way to disable the auto-collapsing? We've had issues with Actions failing to run or delaying start, so I've built up mental memory of "no green checkmarks == something is wrong". Collapsing everything here makes it look, at first glance, like nothing has run, which is a problem that needs to be addressed. I would much prefer to have the option to keep all sections expanded here |
Beta Was this translation helpful? Give feedback.
-
The sorting is braking. little bit when using matrix with numbers.
|
Beta Was this translation helpful? Give feedback.
-
|
Totally agree. We have an internal issue tracking this. I'll add it to the list of Known issues > Planned changes above. |
Beta Was this translation helpful? Give feedback.
-
|
@genintho - can you give this another try? We rolled out a fix today to order status checks more logically. |
Beta Was this translation helpful? Give feedback.
-
|
@willsmythe it looks fixed! Thank you! |
Beta Was this translation helpful? Give feedback.
-
|
The details button to see the tests running is gone, which make life more complicated. I can't see a reason to kill it. |
Beta Was this translation helpful? Give feedback.
-
|
New experience is too alarming, everything is red all over the place. I mean ok, I need to get the reviews, please don't call the cops and the firemen. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks. A fix is in the queue. |
Beta Was this translation helpful? Give feedback.
-
|
New feature request for future interaction of improvements: |
Beta Was this translation helpful? Give feedback.
-
|
As long as I don't have to see a red 'x' because I don't have merge permissions for specific codebases this is fine, I don't like it because it makes it seem like something is wrong when it isn't. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for reporting. Are you seeing this on a PR in a public or private repo? Do you have any access/permission (even just "read" or "triage") in the repo? |
Beta Was this translation helpful? Give feedback.
-
This appears to be a lie: Changes can be cleanly merged.kubernetes-sigs/gateway-api#3615 
This branch has conflicts that must be resolved geps/gep-1651/index.md jsoref@jsoref-mbp gateway-api % git branch -f main jsoref/main
branch 'main' set up to track 'jsoref/main'.
jsoref@jsoref-mbp gateway-api % git rebase main
Auto-merging geps/gep-1651/index.md
CONFLICT (content): Merge conflict in geps/gep-1651/index.md
Auto-merging geps/gep-1713/index.md
CONFLICT (content): Merge conflict in geps/gep-1713/index.md
Auto-merging geps/gep-1742/index.md
CONFLICT (content): Merge conflict in geps/gep-1742/index.md
Auto-merging geps/gep-1911/index.md
CONFLICT (content): Merge conflict in geps/gep-1911/index.md
Auto-merging geps/gep-2659/index.md
CONFLICT (content): Merge conflict in geps/gep-2659/index.md
error: could not apply 23cc738... link: status definitions
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Could not apply 23cc738... link: status definitionsdiff --cc geps/gep-1651/index.md
index 6808296,b2dfe68..0000000
--- a/geps/gep-1651/index.md
+++ b/geps/gep-1651/index.md
@@@ -3,7 -3,7 +3,11 @@@
* Issue: [#1651](https://github.com/kubernetes-sigs/gateway-api/issues/1651)
* Status: Provisional
++<<<<<<< HEAD
+((See status definitions [here](/geps/overview/#gep-states).)
++=======
+ (See [status definitions](overview.md#status).)
++>>>>>>> 23cc738 (link: status definitions)
## TLDR
diff --cc geps/gep-1713/index.md
index b0c5dd3,4fc7051..0000000
--- a/geps/gep-1713/index.md
+++ b/geps/gep-1713/index.md
@@@ -3,7 -3,7 +3,11 @@@
* Issue: [#1713](/kubernetes-sigs/gateway-api/issues/1713)
* Status: Provisional
++<<<<<<< HEAD
+((See status definitions [here](/geps/overview/#gep-states).)
++=======
+ (See [status definitions](overview.md#status).)
++>>>>>>> 23cc738 (link: status definitions)
## Introduction
diff --cc geps/gep-1742/index.md
index 5b120c4,7204560..0000000
--- a/geps/gep-1742/index.md
+++ b/geps/gep-1742/index.md
@@@ -3,7 -3,7 +3,11 @@@
* Issue: [#1742](https://github.com/kubernetes-sigs/gateway-api/issues/1742)
* Status: Standard
++<<<<<<< HEAD
+((See status definitions [here](/geps/overview/#gep-states).)
++=======
+ (See [status definitions](overview.md#status).)
++>>>>>>> 23cc738 (link: status definitions)
## TLDR
diff --cc geps/gep-1911/index.md
index 238c5c4,a835a45..0000000
--- a/geps/gep-1911/index.md
+++ b/geps/gep-1911/index.md
@@@ -3,7 -3,7 +3,11 @@@
* Issue: [#1911](https://github.com/kubernetes-sigs/gateway-api/issues/1911)
* Status: Standard
++<<<<<<< HEAD
+((See status definitions [here](/geps/overview/#gep-states).)
++=======
+ (See [status definitions](overview.md#status).)
++>>>>>>> 23cc738 (link: status definitions)
## TLDR
diff --cc geps/gep-2659/index.md
index e3a8ca6,e7bd954..0000000
--- a/geps/gep-2659/index.md
+++ b/geps/gep-2659/index.md
@@@ -4,7 -4,7 +4,11 @@@
* Type: Memorandum
* Status: Accepted
++<<<<<<< HEAD
+((See status definitions [here](/geps/overview/#gep-states).)
++=======
+ (See [status definitions](overview.md#status).)
++>>>>>>> 23cc738 (link: status definitions)
## TLDR
Compare: This branch has conflicts that must be resolved |
Beta Was this translation helpful? Give feedback.
-
|
It seems that the status has changed? This is what I just saw
|
Beta Was this translation helpful? Give feedback.
-
|
Yes, for some reason ( I can't imagine why. Maybe that person didn't want a 🤖 to kill a PR just because there were merge conflicts. And maybe getting the PR merged was more important than a new broken PR experience. |
Beta Was this translation helpful? Give feedback.
-
|
Last time I was opted into this test I opted out because the merge button and the subsequent confirmation button end up in a different spot on the screen, so it requires more scrolling and tedious mouse work compared to the existing system that let me keep my mouse in the same spot. Can we keep the existing behavior when it comes to merge/confirm buttons please? |
Beta Was this translation helpful? Give feedback.
-
|
Would it be possible to restore the old color scheme? The red and especially the "orange" (rather brown) are very dark and look worse than the old colors. |
Beta Was this translation helpful? Give feedback.
-
|
I am having a lot of issues today.. Resolve threads not going through without refresh; something something json stream error when trying to merge main etc. |
Beta Was this translation helpful? Give feedback.
-
|
Use the PR title as the commit message please. Don't default to the first commit message. |
Beta Was this translation helpful? Give feedback.
-
This message is too cryptic/opaque for the reader to:
|
Beta Was this translation helpful? Give feedback.
-
|
I see side text like "Started 1m ago — This check has started..." on running checks (with yellow circles). I think the " — This check has started..." part is redundant since "Started 1m ago" is shown. |
Beta Was this translation helpful? Give feedback.
-
|
Please don't hide the names of the checks behind an ellipsis, or at least allow me to configure it to not put an ellipsis there.
I our case over at nixos/nixpkgs we have some tests where IMO important information is contained at the end of the check names. |
Beta Was this translation helpful? Give feedback.
-
|
When clicking "Update branch", I'm getting I'm getting it everytime and disappears after some time - the merge actually occurs successfully |
Beta Was this translation helpful? Give feedback.
-
|
+1, I observed the same |
Beta Was this translation helpful? Give feedback.
-
|
In a repo where all merging is done with a bot, the new UI is worse than the old one. Before:
After:
The new UI hides the CI details, making it much harder to see the status of the CI jobs. So for rust-lang, the new experience is a strict regression. |
Beta Was this translation helpful? Give feedback.
-
|
"Checking for the ability to merge automatically" |
Beta Was this translation helpful? Give feedback.
and others
-
Tip
Important
About the new experience
To help you better understand the state of your pull request and get it merged faster, the merge experience on the pull request page has been improved! This experience is currently in public preview.
What's new
We've maintained the familiar look of the existing merge experience while incorporating several usability improvements:
How to turn it on (or off)
Click the Try the new merge experience link below the merge box on the pull request page to enable the improved experience:
To switch back to the classic experience, click the Switch back to the classic merge experience:
You can also toggle the experience via the feature preview dialog.
🔴 🔴 🔴 Known issues 🔴 🔴 🔴
You will run into some bugs and missing features (let us know when you do if not listed here).
Missing features
Features currently missing:
Add your reviewaction in the Reviewers section (workaround: navigate to the Files Changed tab and clickReview changes)Bugs
Resolve conflictsbutton results in a 404/Not Found in certain casesThe base branch does not accept merge commits.Recently fixed
Full list
Delete branchbutton is sometimes missing (fixed by reloading the page)Delete branch/Restore branchfails with aCouldn't update branchmessage (likely occurs only in PRs from forks)Restore branchbutton is easily confused forDelete branch(we moved theRestore branchbutton out of the new merge experience so the only button that will appear here isDelete branch)button (to enable auto-merge) is too easily confused forSquash and merge` (to directly merge) because of similar text and styling *FIX IN PROGRESSLooking for feedback 🔈 🔈
Feedback
We want to hear from you! Post your comments, questions, likes, and dislikes below.
Beta Was this translation helpful? Give feedback.
All reactions