After submitting an MR, the page temporarily displays the following message until the pipeline begins running:
After the pipeline begins running, the merge button displays as expected.
This interaction is disorienting, because there are no items to resolve and it causes the user to believe that there is action to be taken when there is not.
Possible Solutions
Don't display a message at all, since the interim state persists for a short time. Just load the merge widget when it's available.
Update the message to accurately reflect that the MR is waiting for the pipeline to begin running.
The current short term solution is to create a message that is less misleading. There is an open MR: !34958 (merged) and so far, the message would be: A CI/CD pipeline must run and be successful before merge. (?) where the ? leads to the documentation on Pipelines must succeed this would be less confusing to user.
The second step will be to add a loading state to this widget. Because we cannot determine if a pipeline is being created or if there is just no pipeline, we can add a loading time of X amount (say 1 minute) where we tell our user we are checking status of their pipeline. If it get created under the required time, they see the normal pipeline widget (so they skipped the error message altogether). If it doesn't, they see the new message A CI/CD pipeline must run and be successful before merge. (?) .
The long term solution would be more in line with exposing more data to the FE about the current state of the project so that we can give meaningful, accurate message for all states, but this is far off in time due to complexity on the API side.
Proposal
Because the problem to solve is a scenario when a pipeline exists and there is a brief delay until it starts running, the proposed remedy is to replace the existing misleading message You can only merge once the items above are resolved with a generic message to prevent the perception of a error that requires further action.
Possible copy for generic message:
Waiting for pipeline to start...
Checking ability to merge automatically...
Expand here for previous Proposal which is de-scoped from this issue and deferred to later iteration in #225609 (closed)
Display a specific message near the “Merge” button if the Pipelines must succeed merge check is enabled.
On page load
“Pipelines must succeed” enabled and pipeline hasn't started or finished
“Pipelines must succeed” enabled and pipeline has failed
Merge button should be enabled regardless of pipeline status, but we need to check whether that’s the case today.
Additional context
This is happening on every single MR and it happens because we have the option Pipelines Must succeed turned on, and the pipeline for the MR is not yet processed. When the MR is created, this goes back to normal.
Some time after this issue was created, we had worked on improving a different state: when a user had Pipeline Must Succeed turned on, but was using no internal or external CI. The message was too generic, so we added a message for that state which said: 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.
This message was supposed to only appear when a user had no CI config, but we realized afterwards that if an MR has no pipeline, even if there is a CI configuration, we cannot make the difference which mean that currently, this is the message people are seeing when creating a MR on gitlab.com.
It sounds like this is issue exists because we haven't defined a UI for this state (MR created, pipeline not started) yet, so the solution would hopefully be to just define a state and make sure it gets displayed under the right conditions.
@pedroms My spontaneous reaction would be that it would be preferrable to create a "Waiting for pipeline to start" state instead of hiding the widget, as otherwise users might get confused if the pipeline does not get kicked off within the first couple of seconds, what do you think?
@mvanremmerden I agree. The MR widget has more information/actions other than the Merge area, so we should try to show it if we can.
I assume that this “waiting” state only occurs if the project has enabled the Pipelines must succeed merge check. We'll have to consult with development to confirm this and see if there are other similar cases.
@pedroms Let's have @v_mishra review the Proposal in issue description since she is our new product designer for ~"group::continuous integration" and @dimitrieh is now working with ~"group::progressive delivery".
@thaoyeager ah, yes, thanks I still think there's value in @dimitrieh helping out here since @v_mishra may not have all of the needed context to understand the effects of the related features. But I'll let you all discuss the most reasonable approach. In any case, I'll try to follow along and help out in any way I can, since this bug affects merge requests.
@dimitrieh this issue seems to be caused by the Pipelines must succeed merge check, which is part of ~"group::continuous integration". Can you please confirm if this is correct?
@dimitrieh It seems like there is another similar issues with merge trains. I"m not sure whether that's also due to Pipelines must succeed, so I have opened a seperate issue for that: #218084 (closed)
@nudalova@thaoyeager Pedro and I investigated this problem, and it seems like this wrong default state is caused by the "Pipelines must succeed" check, could you have a look to see whether that's the case and whether your group has capacity to take this on?
@thaoyeager I believe this issue needs to be broken down into smaller issues to be weighted.
If I understand correctly, the proposal is to:
Add a loading state with appropriate text and icon
Add another state that displays when pipelines must succeed but the pipeline is not currently successful
Confirm that the Merge Immediately button appears when “Pipelines must succeed” is disabled
Assuming yes, I have expect the weight for this to be more than 5, so we should break it down.
This is a 2.
This is a 3. It is possible that we do not get the state we need from the backend to distinguish this case, but it needs more investigation.
This last needs a little more poking around as well, but may be a 1 or even a 0. Looking around in the code seems to indicate it is the case already, but I would like to take the time to confirm it (or someone else could if you didn't want to wait for me).
@thaoyeager I've been looking into this issue and there are a couple of problems that would need to be resolved before we can proceed with development
We don't have a stable way to know if there is no pipeline because it has yet to be created, because it's stuck (runner nor picking it up) or if there is no CI setup at all. This mean that showing the message in state 2 would require some collaboration with the BE team to expose this kind of information. I had raised this problem before, but I think we actually have a larger problem in that part of the code where we can't give a great experience because are lacking data.
The same problem applies to a loading state. There is no real "loading" happening on the Frontend, so to know that we are waiting after a pipeline, we need to know that the project has pipelines, but currently the MR has none associated with it so we can infer that we are waiting after the pipeline. Otherwise, we could easily end up with a message that says "loading" but actually, it's just that the pipeline is stuck for example. That would be worst than the current UX.
I am going to need some guidance on the next step here: do we need to involve BE engineer in 13.2? Is it time to rethink this widget as a whole? I might even be a good candidate to use GraphQL to get access to all the data it requires that might not be from a single endpoint.
I think getting this information from backend would be great, but we probably don't need it for this. The Merge is only allowed once the pipeline has passed messaging works for when the pipeline hasn't started, is stuck, or isn't even set up. The only information we really need is whether the Pipelines must succeed merge check is enabled.
As for the loading state, we could display it until we know if that check is enabled or not then go from there. This would prevent it displaying a loading state when the pipeline is stuck.
Having some GDK issues at the moment so I can't dig in to see if that check is exposed or not, but it should be the only one you need. Let me know if I'm way off here, this part of the codebase is still new to me.
A couple of milestones ago, I implemented this: !31112 (merged) which is a way to tell the user that if they have no CI setup at all and they have the option Pipelines must success option turned on, then they cannot merge as these are conflicting settings.
What I didn't realize at the time was that this logic was based on an API property hasCi which was supposed to say if a project had any CI configuration at all (either with GitLab or an external service). However, it turned out that if the pipeline is stuck, then this value returns false. So currently, when the usecase described happen (the pipeline is expected to run, but it's not yet created) it's this message that is getting shown while it's "loading" because there is no CI and the Pipelines must success option is true. It seems that this issue might have been created before my MR got merged (at least when I test it locally, that's the behaviour I am seeing) So we have no way that I know of to differentiate this "loading" state that we want from the "your configuration is invalid" state.
This is why I was wondering if we could expose the information to differentiate hasCi (which really should tell us if the project has CI, not if there is a pipeline) and is there currently a pipeline. That way we could make these 2 states coexist properly. Does that make sense? It's a bit of a weird one, so I am just happy to have someone else looking at this with me
As a side note, the check for whether or not the Pipeline must Succeed is enabled, AFAIK, as soon as we get in the MR, it's available so there would be no delay BUT I could double check to make sure.
Just to summarize some Slack discussions in this issue for transparency: we discussed that while it would be helpful to mock possible responses so that FE can begin work, having a quick sync between FE and BE would allow us to collaborate on this problem together, which helps to inform priority and scheduling as well. Ultimately we need to understand what data is available or missing from the BE, and have some backend input around what adjustments (if any) need to be made in the endpoints.
@fabiopitino Can you take a look at what @f_caplette captured above and have a quick sync to see what BE work is needed here? Then I think we can ask for a weight from both of you to help inform Milestone commitments.
After looking a bit at the response from the API and knowing we will have a sync with FE and BE, I am writing clear questions that I would like us to answer during our discussion.
hasCi property returns false when the pipeline has not started yet even if the project has CI configured. Is this intentional?
If it's intentional, how can we know that project has no CI configured? This is needed to tell a user that there might be a conflict if they don't have any CI and they have Pipeline must succeed option
If it's not intentional, then how could we fix this? What is the repercussion on the code base and what would need to be fixed and changed BE and FE?
To determine that a MR is going to have a pipeline, the initial idea was to ask if(hasCi && noPipeline) then we show a loading state. Does that seem reasonable or do we have something more solid on the API side so that we know we are currently processing a pipeline? (ideally, not more polling)
I think this should help guide our conversation and the next steps.
hasCi property returns false when the pipeline has not started yet even if the project has CI configured. Is this intentional?
hasCi returns true if:
the MR has commits AND
there are pipelines created for the given MR OR project has external CI provider setup
This would return false if GitLab pipelines have not yet been created. Given that when we create a merge request, the pipeline is created async there are high chances that hasCi can return false in the meantime.
Is this intentional?
I'm not sure. The latest changes to hasCi are from 3 years ago. I believe it's just a definition not up-to-date with the current feature set.
how can we know that project has no CI configured?
For GitLab CI we could run the same checks that we currently do when creating a pipeline, which is finding a YAML file based on the CI configuration path. This however is not something that we can have statically defined but it needs to be checked all the time and it's not cheap given that it involves Gitaly calls. For example:
A file .gitlab-ci.yml may not exist on master yet but it's being introduced in a new branch add-ci-config. On master, hasCi should return false while on add-ci-config branch should return true.
the .gitlab-ci.yml file may be present in one commit of the branch add-ci-config but not in the latest commit. In this case hasCi would return false for the latest commit.
This starts already to sound complex and it duplicates checks that are already running asynchronously as part of the pipeline creation.
Could we leverage the Pipelines must succeed setting? If enabled then we need to wait for CI. This is the fix that we have used in the past to define if a MR is "mergeable" from CI point of view. In that case if Pipelines must succeed is enabled we expect a pipeline to be present for the latest commit of the MR. If the latest commit is not present, the MR is not mergeable and we need to wait for the pipeline to be created.
This change was necessary especially to align the UX with the external CI providers. In this case, a CI provider may communicate to GitLab after quite some time that a pipeline has been initiated. In the meantime we didn't want to make the MR mergeable from CI perspective. If a pipeline wasn't created, the MR is not mergeable.
Could we align the definition of has CI with Pipelines must succeed project setting? If Pipelines must succeed then hasCi immediately returns true. This invariant is also maintained when we check further if a MR is mergeable from CI perspective.
Prefacing this message to say that Pipeline must succeed option is replaced with PS throughout the text for convenience.
@fabiopitino Thanks for the clarification. I think then that as you said, it's probably not worth it to have queries that involve Gitaly given it's not a cheap operation. As for your proposition, if PS is active and that makes hasCI return true immediately, I think that's a promising option. However, it might cause some other problems to arise.
We have a weird conflict in the UI where Pipelines must succeed can be true, but hasCI is false. This is because for a user that doesn't use any CI and accidentally has the PS option on, they will get a blocked merged button ... forever (At least until they turn off the PS option).
So we implemented some logic so that we can tell the user when there is a conflict in these settings by basing it on hasCI being false and PS being true so it expresses: "No CI is present,but all CI are required to pass before merging which is not valid" If we change this behaviour so that hasCI always return true if PS is true, then we cannot differentiate a configuration error from a normal transition state. The MR will become mergeable for the case of this issue, but remains un-mergeable forever if the user has no CI at all.
Though now that the hasCI conditions has been clarified, there is a shortcoming with this approach anyway because if the pipeline is getting created, then hasCI is false so we show the configuration error message. There might be value to address this configuration issue in another, more robust way of preventing this from happening, but that'd require some investigation from UX.
So my question is, if PS is enabled, we want to show a loading state on page load and transition to a state that allow the user to merge when the pipeline succeed, BUT we also want to retain the ability to tell the user that their configuration might be invalid, we either:
Need a value that tell us a user doesn't have a config, which we determined was too expensive
Prevent the user from ever getting in that state, and then we can move hasCI to always return true if PS is enabled
Something else that I can't think of
I also need some UX clarification on the requirements as really, I think what seemed to be a straightforward solution is hiding complexity. Summoning @dimitrieh as the expert here
With the proposed changes, we want:
A loading state if PS is true on load regardless of pipeline status
A transition state if PS enabled and pipeline hasn't started or finished
A state for success and failed pipelines
I am a bit confused by this because of all of the above, but also:
When we know if PS is enabled, we also know if no pipeline has begun. Do we really want a loading state instead of directly going with Merge when pipeline succeed state?
I would have thought that a loading state while no pipeline is present would have make more sense. I think the confusion comes from #2 (closed): and pipeline hasn't started which I think is not what we want. We either want to directly jump to merge when ready, regardless of if the pipeline has started or not OR we add a loading while the pipeline is getting fetched and once we have one, we enter the transition state for as long as the pipeline is running.
@f_caplette I wonder if a useful solution might be to let people solve what is hard for computers to solve.
That is, what if we unified the messaging so that if Pipeline must succeed is checked and hasCi is false, we showed an Unable to merge till pipeline succeeds and included extra text to explain that this could be because there is no CI, because external CI hasn't contacted us, or because we are waiting for the runner to start the pipeline.
The user probably knows which is the likely case or how to go find out better than we do.
So then we'd have three states: must succeed + success; must succeed + pipeline exists but not successful (running, failed, canceled, etc.); must succeed + no pipeline yet.
Another option might be to check to see how long ago the pipeline was created and not show an error until >1 minute or something, to give it a chance to process. Defaulting to an error message until proven otherwise feels wrong, though.
I like your idea a lot too @sarahghp - just show something more informational and it becomes a non-issue. A message here is ok, but a misleading one that could be interpreted as an error not so much.
So then we'd have three states: must succeed + success; must succeed + pipeline exists but not successful (running, failed, canceled, etc.); must succeed + no pipeline yet.
@sarahghp aren't those states already reflected on the proposal in the issue description? Are we missing something there?
@pedroms As per my comments above,there was a combination of states that were not taken into account. @sarahghp comment addressees that extra state by proposing we merge it with the one you were proposing.
if Pipeline must succeed is checked and hasCi is false we showed an Unable to merge till pipeline succeeds and included extra text to explain that this could be because there is no CI, because external CI hasn't contacted us, or because we are waiting for the runner to start the pipeline.
What do you think of the proposition here?
Also, my other question was if we have a loading state until we find a pipeline, what's the timeout? In some case (as per my comment also) it will never find a pipeline. Do we timeout after a minute like @jyavorska proposed? (I think that make sense) and then go with @sarahghp proposition of having a generic message that encompass both "No pipeline found yet" and "It might be because your PS is enable".
@thaoyeager I saw your comments below and answering here to consolidate the conversation! I think that this would be ideal:
1 MR to add a generic message that removes the perception of an error (this would be pretty sure to make 13.2)
1 MR to add a loading state if no pipeline is found on load for X # of time, then transition to the new state added in step 1. (This might make 13.2, but it might depends on my other deliverable, so we would have to discuss it)
If we want to add a "Merge when succeed" in the error message, I would suggest that this come as a further iteration and start by tackling the ~bug part of this issue.
Does that seem reasonable? The following plan would involve no BE work (thanks @sarahghp).
@f_caplette That sounds great! I'm gonna update this issue's description to scope it specifically to achieve item 1 in your list and add issues for items 2 and 3 so we have separate issues as we iterate on this UX improvement. Can you tell me if both the update to the text and the icon are achievable together?
@thaoyeager When you say the update to the icon, do you mean the loading icon? If so that would go with item #2 and then yes, item 2 includes having a load icon (which we already have in our UI library) and updating the text.
@f_caplette I agree with this decision as it also matches the behavior of Pipeline Must Succeed with external CI providers. When a pipeline is created by (let's say) Jenkins, it can appear on GitLab after it finishes in Jenkins, so you can imagine this having a very long delay. In the past we weren't expecting the presence of a pipeline when using Pipeline Must Succeed and it caused problems because a MR could be merged while the pipeline was still running in Jenkins.
To fix that problem we decided to make Pipeline Must Succeed strictly expecting a pipeline to be created. I feel that the problem with external CI is an exaggeration of this current problem where the delay may be much shorter.
@thaoyeager That makes a lot of sense, seems like what's happening behind the scenes is a bit more complex than it initially looked like. Thanks for making sure it gets addressed
@f_caplette What would be the effort just to change icon and the text of the message on page load (even if checking for "Pipeline must succeed" has not started); here's the icon and text change I'm proposing:
EXISTING:
PROPOSED CHANGE:
I'm hoping the smallest change can be delivered in the current milestone 13.2 and we can continue in next milestone changes to handle the other states.
@ebrinkman@jyavorska If the purpose of bumping to ~P1 is to ensure some/any improvement is made in the current milestone (a minimal change), I can support this. But I don't think this misleading message that temporarily displays is at the level of an ~S1 Blocker because momentarily once the pipeline begins running, the merge button displays as expected.
When I am trying to reproduce this issue, I am seeing the following while the pipeline is not created:
As you can see, the message is different than what is shown in the issue description. @jramsay you mentioned seeing this in every single MR, could you tell me if you are also seeing this message or the one shown in the issue description? This will help me in my investigation, thanks!
@f_caplette I think the message in the issue description with text You can only merge once the items above are resolved. is the one seen in every single MR based on the context of the rest of the problem statement in the description that After the pipeline begins running, the merge button displays as expected.
@f_caplette And that being said, the scope of this issue is to address the scenario of the message in the issue description and I will create a separate issue for the scenario when this message appears:
@thaoyeager But that's the thing I am not sure about: In the case of the screenshot I am showing, the message will also change to be the normal merge enabled as soon as the pipeline is created .
After this issue was created, I implemented that message for the same conditions that we want to test for (no CI, with pipelines must succeed) and so normally, this is the message that we should be seeing when the pipeline is created. If it's not, then I would be a bit confused as that's what I am seeing when I create an MR, so the source of the problem could be different.
That's why I really need someone to tell me if, since we created this issue, they still see the same message. Because I introduced code that should have change this behaviour to be the screenshot I am showing, even if accidentally. Could we confirm with some internal GitLab users?
@f_caplette I have update this issue's description to limit its scope to item 1 of your comment and I have create a separate issue #225609 (closed) to address item 2 (and possibly item 3) in your comment.
I have not added this new issue #225609 (closed) to the current milestone 13.2 because I want to give @v_mishra an opportunity to design the UX before we proceed on that issue.
@thaoyeager thanks for making a quick change to update the language, I've definitely noticed it this week.
Personally I find the phrase we are using, which is Checking ability to merge automatically, to be slightly confusing with most of the confusion stemming from the word automatically. I think this could be interpreted to users that the MR will be merged automatically, rather than the intention was that we are automatically checking to see if it can be merged.
What if we just dropped the word automatically? Checking ability to merge seems pretty clear to me. What do you think?
@ebrinkman Actually, the "automatically" word is not for communicating that a check is automatically being made; it is literally referring to ability to "merge automatically" and a validated check would display a button to Merge when pipeline succeeds which is a merge that automatically occurs without further user action if pipeline is successful.
@pedroms I think the message copy originated from you so please correct me if I am wrong.
Thanks for ping @ebrinkman. I am unsure when the language was introduced, but I agree the term automatically may impress upon users that something will be done without intervention.
@ebrinkman working is generally a collaboration between product/UX. I don't know that we've designated a DRI for it specifically. Since this is MR related I prefer to be DRI for this.
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.
Going by one of @marcel.amirault 's recommendation, to cover all the use-cases listed above A CI/CD pipeline must run and be successful before merge. (?) would work well as a generic message. In the case of Pipeline must succeed is checked and hasCi is false, users would be directed to relevant section of documentation through the (?) link.
^@thaoyeager@f_caplette
In the case of Pipeline must succeed is checked and hasCi is false, users would be directed to relevant section of documentation through the (?) link.
In this case, what the relevant documentation link is is not clear because it could be a few causes. That's why here we discuss giving the user a list of options about what might be wrong. What do you think about the proposal to list the options in the message?
@v_mishra Wouldn't it make more sense to list the options in the message itself than link to documentation? It always feel slightly unkind as a user to be ejected into documentation, to me.
(Anyways, this is more or a conceptual question that shouldn't stop process, but I was curious. )
@sarahghp I agree with your point. Working through documentation could be frustrating in this scenario given the cause couldn't be specified.
Design proposal > When users hover over the (?) a popover appears with the list of probable causes. The documentation should be linked out for the respective causes from the list itself. I'd be happy to provide a mock-up if needed once the content has been finalised.