Resolve "Misleading message displays when MR request is first submitted"
What does this MR do?
This is the first part of a 2 part improvement to make the MR merge widget better reflect the reality of the process when pipeline must succeed
option is turned on.
MR | Changes |
---|---|
![]() |
Change the misleading message in ready_to_merge widget to be clearer |
[Part II] | Add a loading state to the ready_to_merge widget so that we show it as checking availability for one minute before showing the generic message implemented in part 1. |
Currently the issue we are seeing is if a project has Pipeline must succeed
option turned on, when the MR is created, there are no pipelines yet (being processed by the runner) and so we get a cryptic error message that we are "unable to merge, please resolve the following items". So we want to implement a message that says that we are waiting after the pipeline. This however is not easily doable as we can't differentiate if:
- A project has internal CI setup, and we are waiting after the pipeline
- The project has external CI setup, and we are waiting after the pipeline
- The user has a settings conflict where
Pipeline must succeed
is true, but there are no CI configured at all.
Because of this, we opted for a generic message that list why you might be blocked (either you are waiting after the result of your pipeline, or your configuration is invalid).
We had discussed adding some BE logic to allow us to know if there is a gitlab-ci.yml
file in the project to give more accurate message, but that was a very expensive operation so we decided against it (see: #216048 (comment 367467054))
Issue: #216048 (closed)
Chart for conditions related to ready_to_merge
widget
Screenshots
Before
After
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
Test that the states are all properly covered in unit tests.
Merge request reports
Activity
assigned to @f_caplette
- Resolved by Andrew Fontaine
@marcel.amirault Calling your superhero talent to the rescue
You can check the MR description for a summary of the explanation or reference the main issue: #216048 (closed). The short version is: we want to have a generic message that just tell people that sincePipelines must succeed
is turned on, you can't merge if the pipeline isn't successful. This message should be generic enough so that it includes the following use cases- I just created an MR, but the pipeline has not been created yet
- I have external CI, so I will only hear back from the service later
- I don't have any CI, but somehow I can't merge, why is that? (Because they
Pipelines must succeed
turned on, but no CI)
We had worked together not so long ago on the last use case, and we had come up with:
Pipelines must succeed for merge requests to be eligible to merge. Please enable pipelines for this project to continue. For more information, see the documentation.
Now we have to replace this message to includes all 3 cases. I came up with:
Current configuration requires pipelines to succeed for merge requests to be eligible to merge. For more information, see the documentation.
I liked adding
Current configuration
as I feel this is pretty telling for a user who did not expect the MR to be blocked while not confusing a user who has that setting. It becomes a bit wordy though Maybe it would work just as well without?Pipelines must succeed for merge requests to be eligible to merge. For more information, see the documentation
I think this works pretty well. If I am in one of the 2 first cases, I understand that this mean my pipelines has not passed yet and if my configuration is invalid, I get a link to the documentation which can trigger the user to realize the configuration error.
WDYT? I am open to any other suggestion
Edited by Frédéric Caplette
assigned to @marcel.amirault
added Technical Writing UI text twdoing labels
unassigned @marcel.amirault
added 1674 commits
-
4c464c81...81be8aed - 1674 commits from branch
master
-
4c464c81...81be8aed - 1674 commits from branch
added 1 commit
- f1bdcdc0 - Update ready_to_merge widget copy when no CI found
2 Warnings This MR has a Changelog file outside ee/
, but code changes inee/
. Consider moving the Changelog file intoee/
.You’ve made some app changes, but didn’t add any tests.
That’s OK as long as you’re refactoring existing code,
but please consider adding any of the ~”backstage”, ~”tooling”, ~”tooling::pipelines”, ~”tooling::workflow”, ~”documentation”, ~”QA” labels.Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited, or the chosen person is unavailable.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.
Category Reviewer Maintainer frontend Tristan Read ( @tristan.read
) (UTC+12, 17 hours ahead@f_caplette
)Ezekiel Kigbo ( @ekigbo
) (UTC+9.5, 14.5 hours ahead@f_caplette
)If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by 🤖 GitLab Bot 🤖Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits c30a44e4 and c7db3b04
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 4.02 MB 4.02 MB - 0.0 % mainChunk 3.12 MB 3.12 MB - 0.0 %
Note: We do not have exact data for c30a44e4. So we have used data from: b03f348f.
The target commit was too new, so we used the latest commit from master we have info on.
It might help to rerun thebundle-size-review
job
This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerEdited by 🤖 GitLab Bot 🤖mentioned in issue #216048 (closed)
mentioned in issue #225913 (closed)
changed milestone to %13.2
- Resolved by Thao Yeager
@v_mishra @thaoyeager @marcel.amirault For this MR, the goal is to add a clear generic message that feel less like an error when there are no pipelines with your MR. This is mainly so that when you create a new MR, you get more clarity as to what is going on. This is the new copy that show will show user when the pipeline is does not exist for a MR:
Clicking on the
?
currently leads to the documentation onPipelines Must Succeed
. In a subsequent MR, I will update this to use a new doc page that I am working on where we will add documentation for each error message that can happen in these widgets.The loading state will be implemented in a following MR, hopefully this week as well. If we are all okay with this, I will send this MR to be reviewed by other engineers. Please leave comments in this thread or approve the MR if you are okay with this solution. Thank you!
Edited by Frédéric Caplette
assigned to @v_mishra, @thaoyeager, and @marcel.amirault
unassigned @thaoyeager
assigned to @thaoyeager and unassigned @v_mishra
unassigned @thaoyeager
unassigned @marcel.amirault
added twfinished label and removed twdoing label
@shampton Could you review this MR to update the copy of the MR widget when there are no pipelines? Thanks!
assigned to @shampton
@f_caplette LGTM
unassigned @shampton
@afontaine Could you be the maintainer or this MR? thanks!
assigned to @afontaine
Awesome @f_caplette! I am looking forward to part 2
enabled an automatic merge when the pipeline for ee84e82a succeeds
mentioned in commit f4264d70
added workflowstaging label
added workflowcanary label and removed workflowstaging label
added workflowproduction label and removed workflowcanary label
mentioned in issue #346310