This is description of proposal for implementing first-class support for Pipelines to be executed in context of Merge Requests with exact steps to do and expected behavior.
How it works now?
Currently when we do
git push we create a pipeline for a branch.
When we create a merge request we look for a pipelines created
for that branch in that source project.
The CI builds are always executed in context of source project (fork). The fork needs to have it's own runners that would be executing these builds.
How it would work?
We would always create a pipeline for a merge request in context of target project.
We would make these CI builds for merge request to be run using target project's runners.
We would limit the
Secure Variables exposed to CI builds for merge requests.
We would still allow to execute builds for branches and merge requests.
This behavior could be limited with
We would fire pipeline creation when:
- a new merge request is open,
- a merge request is updated.
We would leave that up to the user to limit when pipeline is executed.
Let's look at current way of running CI builds for merge request:
test: script: echo Hello World only: - branches
The new way would require to use a special
test: script: echo Hello World only: - merge-requests
The above example would create a pipeline only on
The defaults for
only: would stay the same as today:
- Run on every tag,
- Run on every branch,
- Run for every merge-request (as it is now).
To limit a number of builds, ex. to make them to run only on merge-requests or on branches this syntax could be used:
test: script: echo Hello World only: - master - merge-requests
Why doing that?
This allows us to solve a number of problems:
- We run merge requests in context of parent project, making it possible to use a runners of this project.
- It allows us to show the pipelines that were created specifically for merge requests,
- It allows us to limit when we actually create a builds (specific branch, or only for merge-requests) (customer requested feature),
- It allows to create a highly optimised and secure runners infrastracture that would be used to run all community contributions (ex. GitLab contributions),
- It allows us later to introduce triggers that would be executed on merge request actions: creation, update, closing. Allowing us to extend
review appswith being closed on merge request.
- It allows us later to introduce tests on merge results. Thus allows to require testing against latest master. If the pipeline succeeds and it get merged we could introduce an optimisation to the process that we would not run the pipeline on master for that merge request.
- It allows us to show the merge request link in a number of places, like: list of pipelines, list of environments.
How builds on runners would be executed?
This changes the approach moving merge request builds execution from source to target project of merge request. This makes it a little more complicated and requires us to introduce a new option for runners configuration. The same option would be available for
Option 1: Runner access-level
Make runner accessible by user level: every pipeline is executed by the user. We also do look at permissions of the user. This system makes it possible to make runner to be useable only by specific user-level in context of that project.
Lets assume that we have an option:
User access level (I don't really have a good idea about name for it yet).
This option can have access levels of the user:
If administrator would configure runner to be used by
Only developers (members of the project) could use this runners.
If developer does create a fork, but it's still developer member of the project
he could use that runner to create builds.
If user is not developer of the project, but this project is public and he creates a fork, from perspective of this project he is considered to be guest.
To make it possible to run merge request builds from forked projects the user would have to be
This is interesting, because then it actually allows to limit runners to be used only by
As an outcome of that change allowing to limit who can do deployments to target system
if we assume that only one of the specific runners is configured to access production servers.
Option 2: Explicit options
Since the Option 1 can not be very clear how it works there's also a different approach that is much simpler to understand.
We could introduce two options that would hide complexity of
Allow to run merge requests from current project(default:
Allow to run merge requests from forks(default:
- We create a new pipeline when a new merge request is opened,
- We create a new pipeline for every merge request that gets updated by push,
- We create a pipeline in context of parent project,
- We make runners to be aware that they are executing pipeline for merge request,
- We add an option to limit which runners can be used for merge requests: when source_project is the same as target, and when these are different,
- For now we don't pass Secure Variables to builds for merge requests,
- We add
- We introduce
- We extend
Migrating Merge Requests
We would store in
MergeRequest information that a pipeline got created with
a new workflow requiring to use
only: merge-requests if
only: is specified.
We would leave the previous approach of finding
merge_requests for past pipelines
so they behavior would be backward compatible.
All new pipelines would require to use
merge_requests after doing system update.
This is mostly great. Running merge request pipelines in the context of the parent is great. I have a couple concerns, and some suggestions.
- We should let project admins specify whether forks should have access to project variables. While this can be a security breach for some projects, sometimes that variables are just configuration and no credentials, so this should be allowed.
- We need
CI_MERGE_REQUEST_SOURCE_BRANCHtoo, even if it can be easily derived. I assume it's got the same value as
- Do we really need
Allow to run merge requests from current projectto ever be false?
- By default, as proposed, everyone's pipelines for merge requests would run twice; once for the branch create/update and once for the merge request create/update. e.g. You push a new branch then create a merge request, and you've run two pipelines with the same git SHA. That seems wrong. We can possibly solve this by making the default be
only: [branches, tags]so merge requests aren't run again by default and need to be explicitly requested. A slight optimization would be to detect MR updates and only trigger either the merge request pipeline or branch pipeline, but not both. But still, MR creation is always a separate step from the initial branch update, so that would always be a redundant pipeline. In fact, it seems really dumb that we would ever allow that to happen, even if they explicitly did
only: [branches, merge-requests].
- It seems like the recommended default should actually be
only: [master, merge-requests], but we'd have to wait until 9.0 or versioning in
.gitlab-ci.ymlto change that default.
- Under this proposal, if the person pushing the forked MR doesn't have permissions on the parent, they won't have access to the variables. That's great, but without any further changes, it most likely means their pipelines will just fail. That's bad. :) How would I configure
.gitlab-ci.ymlto robustly handle non-permissioned forks? Specifically, I'm thinking of a situation where the variables are needed for deployment, but not testing, so the test stages pass, but deploy to review app, for example, fails. One way to handle that is to expose fork-ness in
.gitlab-ci.ymland let people exclude forks from the deploy jobs. But that isn't great because the forker may actually have dev permissions on the parent, but just using a fork flow instead of a branch flow. See related concerns on role-specific variables (#23861).
- Under Option 1, the same problem as above occurs, but would result in a stalled pipeline where there's no runner able to run their pipeline. Or perhaps it would get picked up by shared runners, but then fail because the jobs are written requiring something specific on the project runners. Either way, that's bad UX. We shouldn't create a pipeline that can't be run.
- Option 2 may handle the above better, but would need another option to differentiate between forks from people within vs outside the group that owns the project. Or some other permission-derived thing. Maybe that's option 1. Anyway, if there's no runner with the settings set to true, it results in stalled pipelines.
- This doesn't allow non-permissioned contributors to get review apps for their forked MR. That's a shame. Heroku has a checkbox to allow creation of Review Apps for forks and while it might incur costs from running the app itself, it doesn't leak any deployment credentials. I hope that something like service-level variables (#23859) can let us do the same.
I believe that these deployments we can only handle by not using Runner at all. Otherwise, every configuration is user driven so we can't really enforce a secure configuration.
This is very complicated problem to solve and we are far from doing that in that proposal. I believe that it is fine to assume right now that everyone should have developer access on parent project. This would not help in case of external contributors, but it will definitely help us to make our work.
We can introduce explicit
CI_MERGE_REQUEST_SOURCE_BRANCH, I don't see a problem with that.
Allow to run merge requests from current projectwe probably don't need that.
How do you see defaults, to be
tags, branches? It seems that it will break a default case that you have out of box support for merge requests as this would require extra configuration. I don't really have a good way to solve that.
Maybe we could have a hybrid solution to try to use the current approach (branches) and a new approach (dedicated pipelines) if available. This creates a little mess for time being, but we could drop
branchesapproach with 9.0. The not nice outcome that, if we would show the merge request for pipeline we would only be able to support a new approach so this would kind of create a strange behavior.
I agree with Secret Variables. We can make them available for Merge Requests and add the same checkbox as for runners: expose for forked projects.
The other way to name
allow to be used on forkscould be
allow to run/use with untrusted code. Untrusted would be external, from guests external to this project.
The other way to name
allow to be used on forkscould be
allow to run/use with untrusted code. Untrusted would be external, from guests external to this project.
I like that! There's no reason to worry about forks from developers on the parent, the real risk is "untrusted code" so that's exactly what we should say.
@ayufan How about this alternate proposal: When someone creates a merge request with a different project as the target, we already create a ref in the target repo. Treat that ref as a branch, and run regular pipelines on that (pseudo) branch.'
MR as branch
- When MRs are created from branches on a single project, there's no redundant pipeline. There's just a pipeline for the branch itself.
- When MRs target another project, the pipeline is triggered just like a regular branch (with the exception of having runners and variables not accessible to untrusted code/coders).
- No change to existing apps; no worry about backwards compatibility.
- MRs on a single project will trigger the pipeline before the MR is created, thus missing the meta information about the MR such as target branch. We wouldn't re-trigger the pipeline for this.
- MRs across projects will still trigger a pipeline on the source when the branch is created and the target when the MR is created, thus having redundant pipelines. In this case, I'd assume people would disable CI on the fork, but depending on usage, that might not be sufficient. Sometimes people might want local CI on their fork as well.
The above works well for CI tests, but it's not so great around deploys, especially review apps. For example, if you want to create an app only for MRs, then the above isn't sufficient. For that reason, I think we can still add events for merge request create, update, and close; and let people
excepton them. Thus you could run tests on branch updates, but deploys on MR create/update. We'd have to be careful about ordering, and maybe reflect both events in a single pipeline. Kind of like manual events just augment a pipeline rather than create a new one. Given that argument, maybe it shouldn't be
only, it should be a
whenclause. That seems workable without resorting to separate pipelines for tests and deploys, but still gives people flexibility.
- Switching from a branch-based to MR-based review app flow is just the addition of a
- I think this is how GitHub+Travis works.
variables: REVIEW_APP: $CI_PROJECT_NAME-mr-$CI_MR_ID test: stage: test script: bin/test.sh environments: - review/*: url: $REVIEW_APP.example.com on_close: destroy-review-app create-review-app: stage: review environment: review/$CI_REF_NAME script: - convox apps create $REVIEW_APP - convox deploy --app $REVIEW_APP when: merge-request.create deploy-review-app: stage: review environment: review/$CI_REF_NAME script: convox deploy --app $REVIEW_APP when: merge-request.update destroy-review-app: stage: review environment: review/$CI_REF_NAME script: convox apps delete --app $REVIEW_APP when: merge-request.close
My concern about above is the ownership of Pipeline. We would be effecitvely injecting events from outside on already finished pipelines, thus making it to switch states, send updates and executing stages that could be previously skipped.
We do something similar now, when working with environments, but it's still doesn't feel the same as we do it in very limited scope.
I do see a potential of having
merge-request.open/update/close, as this is a concept that can be extended for branches too.
My biggest concern is that doing
MR as branchwe don't really solve the original issue, and only complicate it even more by adding external
merge request eventsinjected into pipeline. The original issue was about having a direct connection between pipeline and merge request.
How about doing that, by introducing a
features:) configuration to
experimental: - merge-requests
You would have to enable
merge-requeststo use them the new approach now and have a few months window to migrate, but we would make it default with 9.0.
@ayufan Well, it depends on what you call the "original" issue. :) We have several requests and
MR as branchsolves some of them (such as running CI from forks). But I guess what you're saying is that it doesn't solve things like knowing the target and source branch while running the pipeline. At least not unless we also do
The more I think about it, the less I like the idea of making
merge-requeststhe default. Or at least I think that's not how other vendors do it, so I think there's some risk in it. A lot of companies don't use merge requests and it would suck for them to have to add
.gitlab-ci.ymltreatment just to get their builds to work. Also,
mastershould always be build, so that's a very common "exception". Then what about people using Git Flow where they've got a
developbranch too? They'd have to add an exception for that too.
I think my proposal above is not only the cleanest transition, but a reasonable end-state.
What is the real concern about ownership? Assuming we run the builds using the permissions of the person that triggered it, and if we locked down variables to appropriate permissions, wouldn't this work? It does complicate things, but really, it's already complicated in that manual actions can be triggered by different people. We should already deal with the case that pipelines don't have a single trigger person. And that we're going to have to solve #21911.
Does this need frontend work?
I would like to take part in the discussion.
First of all, as I understand it, much of the thinking concerns the execution of pipelines in the context of the parent project. I understand the interest in open source projects and the fact that you are concerned about this problem through Gitlab. However it should not be forgotten that Gitlab is very used by companies and that they are only very little concerned by this problem and it could be pity to add complexity for a relatively low need. One should not fall into the trap of developing Gitlab around the needs of Gitlab. Then it's a very bad idea to put in default master and MR because some uses Gitflow (and this is the case in the company where I work). I see two solutions:
only: default(to restrict to the default branch). The default behavior would be [default, merge_requests]
- Keep the current behavior [branches, tags].
To be honest I mostly prefer a reliable and clearly defined behavior and having a little more configuration, rather than having strange behaviors. So the redundancy detection solution, I do not like too much.
Do not forget this in our thinking: #8998 (closed)
Thanks for your thoughts, @alexispires.
I understand the interest in open source projects and the fact that you are concerned about this problem through Gitlab. However it should not be forgotten that Gitlab is very used by companies and that they are only very little concerned by this problem and it could be pity to add complexity for a relatively low need. One should not fall into the trap of developing Gitlab around the needs of Gitlab. Then it's a very bad idea to put in default master and MR because some uses Gitflow (and this is the case in the company where I work).
We do have customers interested in the fork workflow and they are sorely missing the ability to execute builds in the context of the upstream project. You are correct, not everyone will be interested in this, but it's still very important that we support the option. As far as I know, we have no plans to remove any existing functionality. Maybe @ayufan can confirm this.
An interesting use case for this feature is publishing assets such as documentation or coverage reports. Of course this has a danger of exposing secret variables to malicious users.
However would it be possible to enable running some jobs that rely on using secret variables manually? That would minimize the risk at a relatively small cost (devs would need to click a "deploy" button before triggering sensitive jobs).
I always assumed something similar was already in place when I looked at https://about.gitlab.com/2016/01/08/feature-highlight-wip/ However it turns out that does not work when you use something like
My suggestion is to enable this by default (no yaml changes required) only if merge request's target branch is not filtered by
@joshlambert I think marking as anything other than Backlog will just be misleading. I'd love to see it by 9.2, but there are a ton of ways that won't happen. You could mark it as coming soon which indicates a 1-3 release timeframe (and indicates to UX that they can start looking at it which should make @dimitrieh happy).