Remove default only:[branches, tags] from CI job configuration
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.