Remove default only:[branches, tags] from CI job configuration
Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.
Problem
Merge Request Pipelines show up unexpectedly. This has happened for different reasons in the past, but persistently enough that we are prioritizing making the process simpler and more intuitive.
Background: Where the unexpected pipelines are coming from
The reason merge request pipelines appear unexpectedly is because our implemented logic is to automatically create a merge request pipeline when a merge request is created:
# app/services/merge_requests/after_create_service.rb
def prepare_merge_request(merge_request)
event_service.open_mr(merge_request, current_user)
merge_request_activity_counter.track_create_mr_action(user: current_user)
merge_request_activity_counter.track_mr_including_ci_config(user: current_user, merge_request: merge_request)
notification_service.new_merge_request(merge_request, current_user)
create_pipeline_for(merge_request, current_user) # <-- No checks or guards here!
or updated:
# app/services/merge_requests/refresh_service.rb
def refresh_pipelines_on_merge_requests(merge_request)
create_pipeline_for(merge_request, current_user) # <-- No checks or guards here!
UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
end
which means that under the hood, merge request pipelines are explicitly and always an opt-out feature. I believe this is counter-intuitive to anyone reading our documentation or using our CI platform, because our documentation describes a configuration process where users add to their configuration, seemingly to opt in, to create a merge request pipeline.
Background: How this feature built to operate that way
What really happens is that we always try to create the merge request pipeline, but in the absence of only/except/rules we silently add a default only policy (branches + tags), presuming to opt out for them:
# lib/gitlab/ci/config/entry/processable.rb
entry :only, ::Gitlab::Ci::Config::Entry::Policy,
description: 'Refs policy this job will be executed for.',
default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY,
inherit: false
# lib/gitlab/ci/config/entry/policy.rb
DEFAULT_ONLY = { refs: %w[branches tags] }.freeze
When a user believes they are opting-in to a feature, what they're really doing is overriding a silent opt-out configuration that we never showed them or they knew they ever had. This is why there was a big dustup when we introduced rules, and had to quickly implement workflow to make them somewhat usable. rules and only have an inherent logical conflict that required us to not use the secret, default only:[branches, tags] configuration.
Proposal
How we can fix this by deleting configuration instead of adding more
I believe we should remove the Policy::DEFAULT_ONLY configuration from the processable configuration entry:
# lib/gitlab/ci/config/entry/processable.rb
entry :only, ::Gitlab::Ci::Config::Entry::Policy,
description: 'Refs policy this job will be executed for.',
- default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY,
inherit: false
and checks explicitly for merge_requests configuration before creating:
# app/services/merge_requests/create_pipeline_service.rb
module MergeRequests
class CreatePipelineService < MergeRequests::BaseService
...
def can_create_pipeline_for?(merge_request)
##
# UpdateMergeRequestsWorker could be retried by an exception.
# pipelines for merge request should not be recreated in such case.
return false if !allow_duplicate && merge_request.find_actual_head_pipeline&.merge_request?
return false if merge_request.has_no_commits?
+
+ test_pipeline = Ci::CreatePipelineService.new(pipeline_project(merge_request), current_user, ref: pipeline_ref_for_detached_merge_request_pipeline(merge_request))
+ .execute(:merge_request_event, merge_request: merge_request, dry_run: true)
+
+ return false unless test_pipeline.valid?
true
end
See my comment below regarding the made-up for how the logic of whether or not to create a merge request pipeline is baked into configured_for_merge_request? method#valid? via EvaluateWorkflowRules.
This implementation is much more feasible now than it was in the past because of the significant refactoring in Pipeline configuration that was done to enable the dry_run feature. Using that, we can precompute the result of CI configuration and know which pipelines will actually be created from a given configuration in a live context.
Why this approach is simple and good
So far we've been having pipeline authors configure pipelines by tricking them into a logically double-negative configuration on their jobs, which implicitly control the pipelines. Where the application is at right now, I think we can change this to be truly opt-in, and not need to add another default workflow configuration to be optionally overridden. The default for only: adds significant cognitive complexity to our CI configuration logic (for our own development community as well as users) in my opinion, and I think we'll be far better off if we can simplify an existing implementation instead of adding another one of the same complexity to intersect with the first.
Outcomes
What pipeline authors see now
| Branch Pipeline Config | MR Pipeline Config | Branch Pipeline | MR Pipeline Created? | |
|---|---|---|---|---|
| Valid | Valid |
|
|
|
| Valid | None |
|
--- | |
| None | Valid | --- |
|
|
| Invalid | Invalid |
|
|
While two pipelines are be expected, they simply duplicate the same error |
| Invalid | None |
|
|
This is the infamous MR pipeline-from-nowhere |
| None | Invalid |
|
|
Less common. If all jobs were configured for MRs, a branch pipeline would show up anyway |
What would happen instead
| Branch Pipeline Config | MR Pipeline Config | Branch Pipeline | MR Pipeline Created? | |
|---|---|---|---|---|
| Valid | Valid |
|
|
|
| Valid | None |
|
--- | |
| None | Valid | --- |
|
|
| Invalid | Invalid |
|
--- | One pipeline persisted with config errors |
| Invalid | None |
|
--- | One pipeline persisted with config errors |
| None | Invalid |
|
--- | One pipeline, theoretically the "wrong" one, with config errors. |
Although we persist the "wrong" pipeline in the final case, it's far less common than the second to last case where we produce an unwanted merge request pipeline which is no longer an issue. Ultimately, we'll never persist two pipelines with the same YAML error on push to an open merge request.
Related Issues
#328850 (comment 566998411) - This simple proposal to add a rule to our template will create a second pipeline on plain CI configurations, because the rules conflict with the default only policy on the first two jobs.