Add CI_COMMIT_FORCE_PUSH variable

What does this MR do and why?

This (untested) change exposes a variable to CI pipelines that allow conditional branching when a force push occurs. The current version of gitlab incorrectly determines the list of files changed when a force push occurs. It would be possible to work around this incorrect handling by giving pipeline authors a variable for branching when a force push has occurred. This MR implements that variable.

• Related to add CI_FORCE_PUSH variable (#409307)
• Related to CI_COMMIT_BEFORE_SHA takes wrong value upon bra... (#357394)

How to set up and validate locally

Configure a .gitlab-ci.yml file with the following:

test:
  stage: test
  script:  
  - echo $CI_COMMIT_FORCE_PUSH

Run the following:

git init
touch foo
git add .
git commit -m "foo"
touch bar
git commit -m "bar"
git push origin main
git reset --hard HEAD~1
touch baz
git add .
git commit -m "baz"
git push origin main --force

Observe that the final push triggers a pipeline wherein CI_COMMIT_FORCE_PUSH is set to "true".

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

• I have evaluated the MR acceptance checklist for this MR.

changed the description

Hey @wescaleout!

Welcome to our community! We're excited to have you here, and can't wait to review this first MR with you!

Thank you for your contribution to GitLab. Please refer to the contribution flow documentation for a quick overview of the process, and the merge request (MR) guidelines for the detailed process.

Did you know about our community forks? Working from there will make your contribution process easier. Please check it out!

When you're ready for a first review, post @gitlab-bot ready. If you know a relevant reviewer(s) (for example, someone that was involved in a related issue), you can also assign them directly with @gitlab-bot ready @user1 @user2.

At any time, if you need help, feel free to post @gitlab-bot help or initiate a mentor session on Discord. Read more on how to get help.

This message was generated automatically. You're welcome to improve it.

added Community contribution workflowin dev labels

assigned to @wescaleout

added linked-issue label

mentioned in issue #409307

mentioned in issue #340132 (closed)

mentioned in issue #349694

mentioned in issue #386274

mentioned in issue #369916 (closed)

mentioned in issue #357394

changed the description

added 1st contribution label

Bit of a hail mary, but @marcel.amirault, can you review this? It's a very small change that will make a significant difference for platform engineers implementing ci-pipelines in monorepos.

changed the description

@gitlab-bot ready

added workflowready for review label and removed workflowin dev label

Hi @marcel.amirault! Please review this documentation merge request. This message was generated automatically. You're welcome to improve it.

added documentation twtriaged labels

requested review from @marcel.amirault

@marcel.amirault, this Community contribution is ready for review.

• Do you have capacity and domain expertise to review this? We are mindful of your time, so if you are not able to take this on, please re-assign to one or more other reviewers.
• Add the workflowin dev label if the merge request needs action from the author.

This message was generated automatically. You're welcome to improve it.

mentioned in issue gitlab-org/quality/triage-reports#12421 (closed)

added featureaddition label

added typefeature label

added grouppipeline security label

added devopsverify sectionops labels

added backend label

changed the description

added Technical Writing docsfeature labels

requested review from @jocelynjane and @morefice

removed review request for @marcel.amirault

approved this merge request

@jocelynjane, thanks for approving this merge request.

This is the first time the merge request is approved. To ensure full test coverage, please start a new pipeline before merging.

For more info, please refer to the following links:

• rspec predictive jobs
• jest predictive jobs
• merging a merge request.

added pipeline:mr-approved label

requested review from @alberts-gitlab

Note to @alberts-gitlab, I have not actually run this code to test the validation steps provided in the MR description. Given the tiny size of the changeset and limited time on my part, I was hopeful that a reviewer who runs Gitlab in development more regularly could validate what I have done.

added 2 commits

• 376c7ea3 - Add predefined variable CI_COMMIT_FORCE_PUSH.
• 349dbdcb - Add expected version number for CI_COMMIT_FORCE_PUSH.

Compare with previous version

added 550 commits

• 349dbdcb...1cc5d94b - 548 commits from branch gitlab-org:master
• 9c24dcdf - Add predefined variable CI_COMMIT_FORCE_PUSH.
• 335476d7 - Add expected version number for CI_COMMIT_FORCE_PUSH.

Compare with previous version

added 2 commits

• 762249ee - Add predefined variable CI_COMMIT_FORCE_PUSH.
• f5af0d2c - Add expected version number for CI_COMMIT_FORCE_PUSH.

Compare with previous version

@morefice, @alberts-gitlab, @jocelynjane, this Community contribution was recently assigned to you for review.

• Do you still have capacity to review this? We are mindful of your time, so if you are not able to take this on, please re-assign to one or more other reviewers.
• Add the workflowin dev label if the merge request needs action from the author.

This message was generated automatically. You're welcome to improve it.

@morefice - please feel free to reassign - not sure what the capacity distribution is on the team.

@alberts-gitlab @iamricecake @dbiryukov

Thoughts about introducing this new variable?

I'd like to get a better understanding of the reason for this suggested new variable:

The current version of gitlab incorrectly determines the list of files changed when a force push occurs.

If this is truly a bug, then maybe we should fix this instead? Or if this is by design and by intention, then we can add the CI variable for special cases depending on what users need. Who would be the rightful group to confirm this behavior?

I agree with @iamricecake.

The current version of gitlab incorrectly determines the list of files changed when a force push occurs.

This new variable seems to be a workaround for the bug referred in #357394.

I'm hesitant to add a new variable simply for a workaround, because it will need to be maintained. I think we should also look at the use case described in #357394 and solve that problem. In that issue, the user uses CI_COMMIT_BEFORE_SHA to get a list of files that have been changed. I tried to get more information in this comment, as to what is the expected list of file change when a force push happens, but haven't got a reply yet.

added automation:reviewers-reminded label

@iamricecake / @alberts-gitlab I can appreciate your shared concern around maintaining another variable over the long term. At the same time, I would like to request that you consider the complexity of the variable itself. It makes accessible a boolean value that is retrieved using a function that is already tested maintained for numerous other purposes. Would you agree it is a fair to say that the maintenance burden is rather small?

As it pertains to fixing the underlying issue with the list of files changed for force pushes, the resolution cascades back to a technical decision Gitlab has already given a lot of consideration to--the decision to set CI_COMMIT_BEFORE_SHA to all zeros on initial pushes to branches. It's my understanding this approach was chosen because without a MR there isn't a definitive source branch to compare to. The question of what to compare to is the same for a force push, in my estimation.

If force pushes set CI_COMMIT_BEFORE_SHA to all zeros, that would resolve the primary use case I need to solve using CI_COMMIT_FORCE_PUSH (controlling the value sent to compare_to to ensure pipelines behave in manner consistent with my client's specific use cases). However, the abstraction would be leaky insomuch as pipeline authors would be unable to differentiate between force pushes and initial pushes of a new branch. I don't personally need to differentiate between them, but I can imagine other pipeline authors might.

@wescaleout After reviewing the Gitlab::Checks::ForcePush.force_push? implementation, I think I'm ok with adding this variable. However, I have one comment on the variable name.

Could you also please add the relevant tests? For more detail, please refer to the test guidelines: https://docs.gitlab.com/ee/development/testing_guide/

@iamricecake what do you think?

Would CI_COMMIT_CHANGED_HISTORY be more appropriate? Technically a git push --force may not necessarily change the history.

I'm aware the code implementation is called Gitlab::Checks::ForcePush.force_push?, but it actually checks for changes in history. We can keep that for the time being, but we should use the more appropriate name for the variable. Once the variable is in place, we cannot change it easily.

Technically a git push --force may not necessarily change the history.

@alberts-gitlab is it actually possible to push force without changing history? When I tried it, it seems it didn't push to remote at all and just responded with Everything up-to-date.

@iamricecake Given the chain A -> B -> C locally, and remote has A -> B, wouldn't git push --force just pushes C as normal?

wouldn't git push --force just pushes C as normal?

@alberts-gitlab oh, do you mean when force pushing a new branch?

removed review request for @alberts-gitlab

removed review request for @morefice

removed review request for @jocelynjane

Hi @jocelynjane

We noticed this MR is marked as workflowready for review but no reviewer is assigned. workflowin dev has automatically been applied to this MR based on the likelihood the review is finished. If additional reviews are still required, please assign a reviewer and reapply workflowready for review.

@wescaleout you may also request a review by commenting @gitlab-bot ready. You can also assign reviewers directly using @gitlab-bot ready @user1 @user2 if you know the relevant reviewer(s), such as those who were involved in a related issue.

This message was generated automatically. You're welcome to improve it.

@wescaleout please review the comments above and update accordingly - then please ping for another review. Thanks!

added workflowin dev label and removed workflowready for review label

removed automation:reviewers-reminded label

mentioned in issue gitlab-community/meta#100 (closed)

@wescaleout, it seems we're waiting on an action from you for approximately two weeks.

• Do you still have capacity to work on this? If not, you might want to close this MR and/or ask someone to take over.
• Do you need help in getting it ready? At any time, you can:
  • ask for help with @gitlab-bot help.
  • initiate a mentor session on Discord.
• If you're actually ready for a review, you can post @gitlab-bot ready.

This message was generated automatically. You're welcome to improve it.

I have tried running the GDK to implement the tests requested several times. Each time I have moved on to other tasks because the provisioning steps of the Vagrantfile fail and I haven't been willing to dedicate the time to debugging why (nor the time required to open an issue describing the failures).

This feature is fairly important from my perspective but the barrier of entry to writing tests is currently too high to justify the level of effort.

If anyone monitoring the comments of this thread already has a functioning development environment I would deeply appreciate their willingness to confirm the code works and to drop a few tests in. The level of effort for actually implementing tests should be incredibly small.

Please leave this MR open. I will get to it eventually if someone else doesn't first.

@alberts-gitlab could you help Tyler out with tests here?

Would still love to see this feature land. Can you please help @alberts-gitlab? I know that writing tests for this functionality would be a very, very easy job.

Unfortunately I am still unable to commit the time to getting the local gitlab development environment running.

Hi @wescaleout I'm following up this and see if you still need time or help to write tests? Here is some information that might be useful:

• Here is a similar MR that adds the variable $CI_PIPELINE_NAME.

  I think probably you will need to add the tests to spec/lib/gitlab/ci/variables/builder/pipeline_spec.rb and commit the change to gitlab.com so that the pipeline can run the tests for you if you're having issues setting up the local development environment (if you are running out of free build time, let me know and I can run the pipeline for you).

• If you are still interested in setting up the local development environment, you can take a look at the GitLab Development Kit (GDK) and follow the instruction to install one.

Let me know if you have other questions, I'm more than happy to answer them for you

Thank you again for your contribution!

Hi @tianwenchen! Thank you for taking the time to reply, sorry for my tardy response.

I would really like this feature to be landed. I've just rebased this branch against the latest master and added what I believe to be a minimal test to validate the presence of CI_FORCE_PUSH.

It's unclear to me in the CI pipelines which job is the one actually performing the test I changed.

If you'd be willing to have a look I would appreciate it greatly!

@iamricecake can you help out this community contribution? We've been stuck on failing tests for a long time now, and for such a small change I'd hate to see this linger in limbo for much longer.

<3 an assist here would be hugely helpful! I am well capable of writing tests but I must admit I haven't been able to justify the level of effort required for getting local development functioning in order to do so. Please, anyone who can actually run the test suite, help!

@wescaleout apologies, I wasn't able to get to this today, will do tomorrow. Will need to refresh my memory on the pending discussions on this MR.

cc @shampton

added automation:author-reminded label

mentioned in issue gitlab-org/quality/triage-reports#13321 (closed)

mentioned in issue gitlab-org/quality/triage-reports#13416 (closed)

added sectionci label and removed sectionops label

mentioned in issue gitlab-org/quality/triage-reports#13480 (closed)

mentioned in issue gitlab-org/quality/triage-reports#13598 (closed)

added idle label

mentioned in issue gitlab-org/quality/triage-reports#13694 (closed)

mentioned in issue gitlab-org/quality/triage-reports#13760 (closed)

mentioned in issue gitlab-org/quality/triage-reports#13776 (closed)

mentioned in issue gitlab-org/quality/triage-reports#13991 (closed)

mentioned in issue gitlab-org/quality/triage-reports#14156 (closed)

mentioned in issue gitlab-org/quality/triage-reports#14363 (closed)

mentioned in issue gitlab-org/quality/triage-reports#14533 (closed)

mentioned in issue gitlab-org/quality/triage-reports#14815 (closed)

added stale label and removed idle label

Hi @shampton!

To provide a better contribution experience, we're identifying older merge requests with no human activity in the past 120 days. Our goal is to bring these merge requests to completion or close them, enabling us to spend more time on active merge requests.

Review this merge request and determine if you should:

• Work on the provided feedback. Add @gitlab-bot ready when you need a review or are looking for more feedback.
• Don't know how to proceed? Ask for help from a merge request coach by adding @gitlab-bot help in a comment.
• Close the merge request.

Please see the handbook for additional details on this

(Improve this message?)

added automation:stale-reminded label

mentioned in issue gitlab-org/quality/triage-reports#14990 (closed)

removed stale label

mentioned in issue gitlab-org/quality/triage-reports#15185 (closed)

added idle label

mentioned in issue gitlab-org/quality/triage-reports#15400 (closed)

mentioned in issue gitlab-org/quality/triage-reports#15660 (closed)

mentioned in issue gitlab-org/quality/triage-reports#15908 (closed)

mentioned in issue gitlab-org/quality/triage-reports#16178 (closed)

removed idle label

mentioned in issue gitlab-org/quality/triage-reports#16364 (closed)

mentioned in issue gitlab-org/quality/triage-reports#16637 (closed)

added idle label

mentioned in issue gitlab-org/quality/triage-reports#16826 (closed)

added 46947 commits

• f5af0d2c...433f7838 - 46945 commits from branch gitlab-org:master
• e2c672ba - Add predefined variable CI_COMMIT_FORCE_PUSH.
• 210451fa - add test detecting CI_COMMIT_FORCE_PUSH variable

Compare with previous version

removed idle label

mentioned in issue gitlab-org/quality/triage-reports#17156 (closed)

The spec is failing because pipeline here is an instance of Ci::Pipeline. But force_push? was defined in a different place (lib/gitlab/ci/pipeline/chain/command.rb). So we have to move it to Ci::Pipeline#force_push? instead (app/models/ci/pipeline.rb).

We also need to add the spec for #force_push? in spec/models/ci/pipeline_spec.rb.

I am talking about this failing spec https://gitlab.com/scaleoutllc/gitlab/-/jobs/6465530976.

@wescaleout I left one comment that would fix one of the significant failing specs. Let me know if you need some help writing specs for the suggestion.

Thanks @iamricecake! I have fixed this failing spec following your suggestions: https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608694056

I would gladly accept help writing the specs you suggest!

@wescaleout looking at failures in the latest pipeline https://gitlab.com/scaleoutllc/gitlab/-/pipelines/1250169305.

For jobs:

• https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608646642
• https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608646717
• https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608646720
• https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608646740
• https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608646827
• https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608646629

It seems we also need to add CI_COMMIT_FORCE_PUSH to other areas of the specs that do check the exact list of variables returned, similar to what you did in line 34 of spec/lib/gitlab/ci/variables/builder/pipeline_spec.rb.

Can you please check the mentioned lines in the failing jobs and add the CI_COMMIT_FORCE_PUSH accordingly?

Now for the failing rubocop job https://gitlab.com/scaleoutllc/gitlab/-/jobs/6608646231:

Offenses:
lib/gitlab/ci/variables/builder/pipeline.rb:44:11: C: Metrics/AbcSize: Assignment Branch Condition size for predefined_commit_variables is too high. [<1, 55, 2> 55.05/54.28]
          def predefined_commit_variables ...
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I feel a bit tricky, as the method predefined_commit_variables, even though might look huge, I am not too sure how we can further simplify it not just for the sake of satisfying the cop. I am thinking we can just disable the cop in this case, something like:

def predefined_commit_variables # rubocop:disable Metrics/AbcSize -- this method is fine as is

@dbiryukov would like to get your opinion about this as well.

Regarding the spec for Ci::Pipeline#force_push?, we can do something like:

# spec/models/ci/pipeline_spec.rb

  describe '#force_push?' do
    let(:pipeline) { create(:ci_pipeline, project: project) }

    it 'checks if it was created from a force push' do
      expect(Gitlab::Checks::ForcePush)
        .to receive(:force_push?)
        .with(project, pipeline.before_sha, pipeline.sha)
        .and_return(true)

      expect(pipeline.force_push?).to eq(true)
    end
  end

By the way, I will be OOO April 15-19. @dbiryukov or @morefice can you please continue to assist here while I am away?

cc @shampton

I feel a bit tricky, as the method predefined_commit_variables, even though might look huge, I am not too sure how we can further simplify it not just for the sake of satisfying the cop. I am thinking we can just disable the cop in this case, something like:

Splitting this seems challenging to me as well, so disabling the RuboCop rule here should suffice.

added 2794 commits

• 210451fa...06c1289e - 2791 commits from branch gitlab-org:master
• a4e7a5f3 - Add predefined variable CI_COMMIT_FORCE_PUSH.
• 15531be6 - add test detecting CI_COMMIT_FORCE_PUSH variable
• df8cd1c6 - move force_push? to Ci:Pipeline

Compare with previous version

mentioned in issue gitlab-org/quality/triage-reports#17363 (closed)

added idle label

mentioned in issue gitlab-org/quality/triage-reports#17618 (closed)

mentioned in issue gitlab-org/quality/triage-reports#17807 (closed)

mentioned in issue gitlab-org/quality/triage-reports#18084 (closed)

mentioned in issue gitlab-org/quality/triage-reports#18260 (closed)

Security policy violations have been resolved.

added devopssoftware supply chain security label and removed devopsverify label

added stale label and removed idle label

Hi @wescaleout, are you still interested in working on this?

removed stale label