Skip to content

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 configured_for_merge_request? method for how the logic of whether or not to create a merge request pipeline is baked into #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 Valid
Valid None Valid ---
None Valid --- Valid
Invalid Invalid Broken Broken While two pipelines are be expected, they simply duplicate the same error
Invalid None Broken Broken (surprise duplicate) This is the infamous MR pipeline-from-nowhere
None Invalid Broken (surprise duplicate) Broken 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 Valid
Valid None Valid ---
None Valid --- Valid
Invalid Invalid Broken --- One pipeline persisted with config errors
Invalid None Broken --- One pipeline persisted with config errors
None Invalid Broken --- 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.

Edited by 🤖 GitLab Bot 🤖