Prevent "Accept Merge Request" if a pipeline hasn't been run yet.

Just wanted to add that this is in regards to a customer ticket... https://gitlab.zendesk.com/agent/tickets/78717

We should fix that.

@grzesiek what's the expected status here? I tried to reproduce with:

test:
  script: echo passed
  when: manual

And the pipeline shows as skipped; is that expected? Is that the same as the example in the description?

We do seem to deliberately ignore skipped pipelines for this setting; probably because of the [ci skip] option in commit messages.

@smcgivern Adding allow_failure: false will make this a blocking manual action, I believe that this is the case.

@grzesiek thanks! In that case I see this

@jwoods06 would you mind creating a public repo to reproduce on GitLab.com?

@smcgivern Interesting! Thanks for trying it! Maybe this behavior has changed after redesigning MR widget. It appears to be correct now. Should we close this issue in that case?

@grzesiek it's possible, but the version info in the description is 9.2.2, so let's wait for @jwoods06 to confirm. It's also possible this was recently fixed in master, or this isn't the case they are describing.

We are experiencing this behavior on v9.2.5 as well. We have the yml to only run CI when triggered or a merge to master has been done.

Attached the yml, couldn't get it to format correctly in pageplayground_yml.txt

@smcgivern Robert is the original reporting customer. Sounds like its still there in the latest release.

Let me know if there's anything I can do to help.

@lasirona @jwoods06 thanks! (The file contains a URL which may be confidential, so you may want to redact that.)

Trying this locally, I get:

So we consider this pipeline skipped, not in manual status. Presumably that's because it can't be triggered from the UI.

@grzesiek what should happen here?

We're getting a different message on the merge request.

If you click the 'Merge' button, the change will still be allowed to merge into master

I get the same merge request widget state that @lasirona does.

The reason it looks like this is that we do not create a pipeline when it has no jobs for given preconditions.

When you use only / except and effecitvely no job needs to be done, we don't create pipeline. And because there is no pipeline for the merge request, we allow to merge it.

@lasirona @smcgivern does it make sense? Is there something we could improve here?

IMHO, if the feature is selected that 'Only allow merge requests to be merged if the pipeline succeeds', that the behavior should default to blocking the merge. Otherwise, IMHO the text for this checkbox is deceiving. Regardless, we would like it blocked so either approach is acceptable. Default to blocking when this checkbox is selected. Or, add as a feature with another checkbox as a sub-feature.

@lasirona that makes sense, but I think how this feature works without a pipeline in general is not very well-defined. For instance, if you broke your CI settings so that you weren't creating pipelines, and wanted to create an MR to fix that, you wouldn't want merging that MR to be blocked ... but I don't know how common that is.

@grzesiek is there a way to work around this? For instance, if you set when: always on another job, does that implicitly create the prior stage, or does it just have the job with when: always?

@smcgivern There is only one workaround for that; always have a least one job in the pipeline for every branch /tag. when is not going to help with that if the job that defines when is excluded for given branch / tag.

@grzesiek right, but in that case, if the defined job passes, the MR is still mergeable, and if it doesn't pass, then the MR will never be mergeable

@smcgivern Yes, the problem is that pipeline has no jobs. @lasirona do we correctly understood your problem? If yes, then there is no good solution, until you add jobs to the pipeline.

@lasirona that makes sense, but I think how this feature works without a pipeline in general is not very well-defined. For instance, if you broke your CI settings so that you weren't creating pipelines, and wanted to create an MR to fix that, you wouldn't want merging that MR to be blocked ... but I don't know how common that is.

We've broken it many times because the controls have to be on every stage. There's always someone who starts a change and forgets one stage (me included). The CI runs based on the current .gitlab-ci.yml, so however that is set. The CI runs according to those settings, so if we have it off and decide to turn it on, it'll run as expected, not based on the previous CI settings.

Besides, if something is completely broken. There's always being able to force push, it just requires the extra steps of unprotecting the destination branch.

Based on the direction of the discussion, I feel like I'm missing something?

@lasirona I think that you suggest that if Only allow merge requests to be merged if the pipeline succeeds feature is enabled, we should require not only pipeline to pass but also that a pipeline should exist. Is that correct?

@grzesiek I think the problem lies more with manual pipelines. If a manual pipeline exists, but hasn't been run, then the merge can be accepted. This is counter-intuitive behaviour to the "Only allow merge requests to be merged if the pipeline succeeds". The pipeline hasn't succeeded because it hasn't been run yet. With that setting enabled, there should be two conditions...

Has a pipeline been run?
Has the pipeline succeeded?

If both those conditions are yes, then the merge can be accepted. If not, then no merge is allowed.

@lasirona I think that you suggest that if Only allow merge requests to be merged if the pipeline succeeds feature is enabled, we should require not only pipeline to pass but also that a pipeline should exist. Is that correct?

Yes, my assumption is such when applying a setting for merge requests to only be allowed to be merged if there is a successful pipeline implies a pipeline result is there to validate against.

@jwoods06 I think maybe a build job should have more than just a pass/fail state. Maybe build jobs should also have a manual state, where this job isn't scored.

@jwoods06 As you can see on a screenshot provided by Sean, merge button is disabled when a pipeline has blocking manual actions. We can also have optional manual actions, but this is not obvious for me that we should block pipeline when the job is optional, and is the only one in the pipeline. In that case pipeline is considered skipped. Is the problem you refer to related to skipped pipelines like when there is only one optional manual action present?

@grzesiek Yes, that's exactly it... if there is a single manual step required, I think that considering it "skipped" is dangerous. It hasn't been skipped, it just hasn't been run yet. Its a big difference (at least, I think so). Maybe its a manual step because there needs to be a discussion first or that kind of thing. There are probably lots of reasons why the step is manual and you need it to be run before you can accept the MR and you want to enforce that. Right now, the feature is called "only allow merge requests to be merged if the pipeline succeeds". Even if we accept the "skipped" terminology, that is not success. Until that pipeline is run and is successful, the MR should not be able to merge, IMO.

@jwoods06 I missed the disabled button, in my browser, the button looks very similar. I'm just assuming that in Sean's screenshot the button is dimmed. As in the yml example I provided, no manual steps in pipeline.

@grzesiek I would have to agree with you. Lumping skipped builds and unrun builds into a single category of accepting them would break other projects for us. We do have some projects that have manual jobs, but no pipelines are run until triggered. In this case, we have some projects that receive dependency libraries from other groups. The manual job allows us to test against those libraries before merging them into mainstream CI. Some are from test docker containers, while others move SHA's in git submodules.

mentioned in issue gitlab-foss#34136 (closed)

@jwoods06 @lasirona Is there a reason why this important manual step can't be a blocking manual action? Do we agree that an optional manual actions should not block the pipeline or merge? Docs https://docs.gitlab.com/ee/ci/yaml/#manual-actions.

@grzesiek as I mentioned before, currently we don't have a manual job in the yaml. I know that in the future we will add it and yes I understand it can't block. What I don't understand is why a merge request with no pipelines run against it is allowed to merge, especially when the CI is configured to only allow successful pipelines to merge. I'm just trying to understand why 'no pipeline' == 'successful pipeline'? Because, that's the behavior exhibited and that doesn't agree with the setting.

@lasirona Thanks for the explanation. I agree that we have some room for improvement here. Making it possible to configure your project the way that CI/CD pipeline is required make some sense. We would need to think about edge cases, but this seems feasible. What do you think @smcgivern?

@grzesiek would we need a configuration setting, or could we just make this one block merges without a pipeline? In https://gitlab.com/gitlab-org/gitlab-ce/issues/33663#note_32981309 I proposed one problem for this, but I don't know how common that is. I think introducing a new setting would be too much overhead.

@smcgivern https://gitlab.com/gitlab-org/gitlab-ce/issues/33663#note_32981309 shouldn't be a problem, pipelines are being created for code on a merge request branch, so it should be possible to push a code there that fixes CI/CD pipeline, thus making it possible to merge it. Am I missing something? We can probably use the same setting to make is possible.

Anyway, let's pass this issue to our product team. /cc @bikebilly

mentioned in issue gitlab-foss#26927 (moved)

This is triggering a problem with creating and setting automatic merge for merge requests via the API. I have a trigger that creates a merge request and then immediately attempts to set merge_when_pipeline_succeeds via the v4 API. This immediately merges the MR, presumably because the CI job hasn't been created yet. See my comment on #26927 for more information.

@bikebilly Do you have any idea how could we improve it?

@grzesiek @bikebilly I don't know if this will break horribly for some people, but I'm willing to try just making MRs without a pipeline unable to merge if this setting is enabled. If you don't have a pipeline on every MR, but want this setting set ... I'm not sure I understand that use-case

@smcgivern I think that would solve my use case, presumably it should still be possible to set merge_when_pipeline_succeeds even if a pipeline hasn't technically been created yet?

@jbrandhorst I don't think so, sadly. The problem is that we don't know if a pipeline will ever be created for the merge request until it is. @grzesiek any ideas?

Hmm, so would the solution to my use case be to sleep for some amount (to allow the pipeline to be created) before setting merge_when_pipeline_succeeds? Presumably such a solution could lead to failures in certain scenarios when the pipeline takes an unusually long time to be created.

Yes, I think polling / sleeping is basically the only option there. I might be wrong though!

I don't have a better idea for now, but I believe that this is something that we should improve.

From a technical point of view, I think that we should know if a merge request is expected to have a pipeline or not, just at the moment that push happens. This would solve a lot of problems. And it should be possible to easily implement that.

@bikebilly how would the waiting time work exactly? I'm not sure I'm clear on that.

@bikebilly ah, I see. I don't think that's guaranteed to be correct, either: if someone has disabled Sidekiq for some reason, or just has a large backlog of jobs to be processed, we may make an MR mergeable that isn't.

I think if we're not going for a comprehensive solution here (https://gitlab.com/gitlab-org/gitlab-ce/issues/33663#note_33750822), then we should pick the absolute simplest option.

Seems like a duplicate of gitlab-foss#28682 (moved) ?

mentioned in issue gitlab-foss#34547 (closed)

I think that in this case we should seek a good solution. The only missing piece of the puzzle is - how could we know if someone will post a commit status in the merge request. Maybe we should have a checkbox like 'Require a pipeline to be present in merge request' (no matter if pipeline is internal or external).

9.4's feature freeze is Friday! Did you mean 9.5, or to fix in 9.4 after the freeze?

Ah, I see @bikebilly I think we should just schedule that for 9.5 unless it's really urgent, but PMs own that anyway.

added SP2 in GitLab FOSS label

Support would love to get this in sooner rather than later, but at least scheduled would be a win in my book. <3

@bikebilly so the plan here is to just do https://gitlab.com/gitlab-org/gitlab-ce/issues/33663#note_33580391? Could you update the description, please?

@bikebilly I'm sorry, I really don't like the idea of a fixed delay for something like this. I think that is more confusing, even if it will break fewer workflows. The scenario is: you visit an MR without a pipeline, and you can't merge it. You visit it N minutes later, and you can merge it, but nothing has changed. It's also a lot of code to write for a temporary fix

Maybe I'm overestimating the confusion here. @grzesiek wdyt?

@smcgivern @bikebilly I think that a fixed delay does not resolve the problem, it makes it even more tricky. You can post an external status to a merge request an hour after you submitted the MR. I think that we should have a setting -> every merge requests needs a pipeline. This might resolve more problems than just this one. I'm not sure if we should use existing setting or introduce a new one. What do you think @bikebilly?

@bikebilly Manual merging is always possible if a user can push merge commits. I agree this is a very tricky and that we should solve it the right way. We can handle internal pipelines pretty well, but the problem is mostly related to external "pipelines". Maybe we should have two configuration options: require internal pipeline and require external commit status?

@bikebilly I think that you are correct

I'm happy with that, as long as we're willing to revert it if (or add a setting) it turns out that people were relying on this case.

@bikebilly OK.

@bikebilly this isn't marked as Deliverable in GitLab FOSS, so my team will get to it once they've handled their Deliverables.

@victorwu this is a Stretch in GitLab FOSS left over from 9.5 (one of only two!). What do you want to do with it?

changed milestone to %Next 2-3 months in gitlab-foss

@smcgivern : Changed the milestone

moved from gitlab-foss#33663 (moved)

Prevent "Accept Merge Request" if a pipeline hasn't been run yet.

Summary

Steps to reproduce

What is the current bug behavior?

What is the expected correct behavior?

Results of GitLab environment info

Results of GitLab application Check

Designs

Child items ...

Activity

Admin message

Admin message

Prevent "Accept Merge Request" if a pipeline hasn't been run yet.

Summary

Steps to reproduce

What is the current bug behavior?

What is the expected correct behavior?

Results of GitLab environment info

Results of GitLab application Check

Activity