FAQ | This is a LIVE service | Changelog

Skip to content
Snippets Groups Projects

Add auto devops merge request pipeline support

Closed Ryan Kowalewski requested to merge add-support-for-mr-pipelines into master
2 unresolved threads

This proposed template overrides all relevant GitLab Auto-DevOps job rules to allow the jobs to run in merge request pipelines. This, coupled with the included workflow rule, enables us to switch between merge request and branch pipelines and avoid the confusion caused by GitLab's annoying duplicate pipelines.

Here is an example of a branch pipeline test (including the currently problematic pre-commit-yml template mentioned in #48 (closed)):

https://gitlab.developers.cam.ac.uk/uis/devops/experiments/webapp-demo/webapp-test/-/pipelines/368078

And here is an example of the exact same pipeline, but triggered as a merge request pipeline (without a duplicate branch-based pipeline) as soon as I opened a dummy MR:

https://gitlab.developers.cam.ac.uk/uis/devops/experiments/webapp-demo/webapp-test/-/pipelines/368081

Adding this workflow is also a blocker on the ongoing semantic-release proof of concept, as the core concept of this tool is that it reviews all commits in a proposed MR to determine that they all follow the conventional commit standard so that it can determine the new semantic version number after the MR is merged.

Finally, I've used the *.gitlab-ci.yml template naming convention in anticipation of #52 (closed) being accepted as worthwhile.

Closes #53 (closed)

Merge request reports

Requires 1 approval from eligible users.

Closed by Ryan KowalewskiRyan Kowalewski 1 year ago (Aug 2, 2023 7:51am UTC)

Merge details

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • assigned to @rk725

  • Ryan Kowalewski requested review from @rjw57

    requested review from @rjw57

  • Ryan Kowalewski added 1 commit

    added 1 commit

    • a1b0912e - feat: add auto devops merge request pipeline support

    Compare with previous version

  • Ryan Kowalewski added 1 commit

    added 1 commit

    • cedee830 - feat: add auto devops merge request pipeline support

    Compare with previous version

    • Although I appreciate that a) this functionality is desirable and b) GitLab appears in no hurry to implement it, I think this is just too brittle to merge as is. It'll break whenever GitLab change any of the existing AutoDevOps rules or, more annoyingly, we'll miss out on any new functionality which is added until the template is updated and all the users move to the newer release.

      As such I think we should instead deal with AutoDevOps running in branch pipelines only. The difference is minor for most things we care about. For example, the likelihood of a merge of a MR into main introducing a new dependency not present in either main or the branch is pretty small and so we don't really need to run dependency scanning on the post-merged result.

      My inclination is to accept the annoyance of having AutoDevOps run in branch pipelines rather than making the rod for our own backs of keeping this template in sync with all AutoDevOps changes.

      Adding this workflow is also a blocker on the ongoing semantic-release proof of concept, as the core concept of this tool is that it reviews all commits in a proposed MR to determine that they all follow the conventional commit standard so that it can determine the new semantic version number after the MR is merged.

      Could you elaborate? AIUI, the only difference between a MR pipeline and a branch pipeline is that the former runs post merge. As such there will be exactly one extra commit present: the automatically generated merge commit which is guaranteed to not correspond to the conventional commit format. The content of the post-merge tree may be different but the commit messages will be identical and so anything linting the commit message format would work equally well in the branch pipeline versus merge request pipeline.

      If the argument against this is "oh, but people want to push branches with crappy commit messages while developing". Fair enough, they can add a commit with "chore: re-word me later" or similar.

    • For avoidance of doubt, this is an active NAK unless we can get a good story together on how we can keep this in sync with AutoDevOps without too much pain.

    • Although I appreciate that a) this functionality is desirable and b) GitLab appears in no hurry to implement it, I think this is just too brittle to merge as is. It'll break whenever GitLab change any of the existing AutoDevOps rules or, more annoyingly, we'll miss out on any new functionality which is added until the template is updated and all the users move to the newer release.

      Agree with @rjw57

    • I think this is just too brittle to merge as is. It'll break whenever GitLab change any of the existing AutoDevOps rules

      I don't believe it is fair to say it will break if GitLab update the job rules upstream. If our defined rule overrides are working for us then changes to the upstream job rules will not affect us, surely? This MR only proposes to override the rules section for these jobs, all other sections (script, variables, image, stage, etc.) are still merged in from the upstream Auto-DevOps templates meaning that any changes that GitLab make to the actual functionality of these templates will be pulled into our pipelines even if using the overrides proposed in this MR.

      As such I think we should instead deal with AutoDevOps running in branch pipelines only. The difference is minor for most things we care about. For example, the likelihood of a merge of a MR into main introducing a new dependency not present in either main or the branch is pretty small and so we don't really need to run dependency scanning on the post-merged result.

      This is the key, I have been thinking we may want to make use of the merged results pipeline option even if not by default. For example, allowing projects to enable the option if required/desired. However, if we agree to only run branch pipelines then we just need to be aware and accept that this means all merge request related options, such as merged results and merge trains, will not be available to our projects. I agree that this is probably a non-issue but I just wanted to note it.

      Could you elaborate? AIUI, the only difference between a MR pipeline and a branch pipeline is that the former runs post merge.

      I was thinking specifically about the additional CI variables that are specifically available in merge request pipelines (e.g. $CI_MERGE_REQUEST_DIFF_BASE_SHA). I had thought that these would be useful. However, on second thoughts this is not an issue.

      Ultimately, I just want to avoid the confusion that these duplicate pipelines cause by agreeing a standard and ensuring our templates all adhere to it and that it's documented in the guidebook.

      So, I'll close this issue off and raise another issue against this repo to check our templates and ensure that they work with the branch pipelines workflow (e.g. the terraform-pipeline.yml currently switches between merge request and branch pipelines so will need amending, and the new python pipelines reference the .rules:disable-except-for-pushes-and-merge-requests rule etc.). Also, I'll raise an issue (or add a comment to a relevant existing issue as I'm sure there'll be one) in the guidebook repo to ensure this is documented.

    • This is the key, I have been thinking we may want to make use of the merged results pipeline option even if not by default. For example, allowing projects to enable the option if required/desired. However, if we agree to only run branch pipelines then we just need to be aware and accept that this means all merge request related options, such as merged results and merge trains, will not be available to our projects. I agree that this is probably a non-issue but I just wanted to note it.

      I'm not suggesting not using MR pipelines, just accepting that the Auto DevOps jobs run in the branch pipeline. Our test jobs, for example, can and should still run in a MR pipeline and provide confidence that the merging into the default branch would not introduce a regression.

      We have a smattering of projects using MR pipelines right now. Certainly I enable them in projects when I remember and don't really encounter any problems. E.g. uis/devops/lib/fastapi-pagination!2 (merged) runs tests both pre- and post-merge and still runs things like dependency scanning as part of Auto DevOps.

    • I don't believe it is fair to say it will break if GitLab update the job rules upstream.

      If GitLab introduce a new job type, for instance "super-amazing detect security vulnerabilities through AI", then we'll just not be running it in the pipeline with the Auto DevOps jobs with no indication that we're missing functionality until someone notices and adds the appropriate rules override. That's the sort of breakage I meant.

    • Or, equivalently, if GitLab update a job to be able to run in more circumstances, e.g. they update container scanning to support scheduled checks, we'll not get that configuration either.

    • OK, I have just tested switching the terraform-pipeline.yml template over to using the branch pipelines workflow rules and it does cause a small usability issue which I'll try and detail below (apologies for the essay).

      TL;DR - If we do standardise on branch jobs only, then we need to document the fact that opening an MR will not trigger a new pipeline.

      Current template

      The current template switches between branch pipelines and merge request pipelines, as I mentioned in my previous comment. What this means is, as you push commits to your feature branch, but before opening a merge request, a slimmed down branch-based pipeline runs containing a few linting/validation jobs, for example:

      https://gitlab.developers.cam.ac.uk/uis/devops/experiments/webapp-demo/infrastructure/-/pipelines/370611

      Then, once you're ready to propose the feature, you open a merge request which triggers a "merge request" pipeline. This pipeline contains a terraform plan job for each environment so that reviewers can easily assess the changes proposed, for example:

      https://gitlab.developers.cam.ac.uk/uis/devops/experiments/webapp-demo/infrastructure/-/pipelines/348864

      This works really nicely, as we wouldn't want plans running for every push to all feature branches, so the ability to only run them when an MR is opened is big plus.

      Branch pipeline only testing

      So, to switch this template to only branch-based pipelines is actually pretty simple, we just need to override the plan job rules as follows (and include the Workflows/Branch-Pipelines.gitlab-ci.yml template obvs).

      .terraform-plan:
        ...
        rules:
          - if: $CI_OPEN_MERGE_REQUESTS
          - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH
          - when: never

      Meaning, if $CI_OPEN_MERGE_REQUESTS is not null include the plan job, or if the commit branch is the default branch include the plan job. The $CI_OPEN_MERGE_REQUESTS variable is available in both merge request pipelines and branch pipelines so this should work. However, what happened was the following.

      When pushing to my feature branch, but before opening an MR, I got the slimmed down pipeline as expected. However, when I opened up an MR the extended pipeline with the plan jobs didn't trigger automatically. This is because the branch-based pipeline relies on a push event, which obviously didn't occur. So, I went into the pipelines tab on the MR to hit the "Run pipeline" button thinking this would run the pipeline with the additional jobs, however, the button is missing from the MR page as we're completely removing merge request pipelines by using the Workflows/Branch-Pipelines.gitlab-ci.yml template.

      For example, this is the Lab Allocator merge request view with the "Run pipeline" button.

      image

      And this is my test repo with the button missing.

      image

      This may seem trivial, however, the alternative is now to go into the main pipelines area (from the main nav), click "Run pipeline", select your feature branch from the drop down menu, and finally click "Run pipeline" again. This then results in the extended pipeline running as expected, for example:

      https://gitlab.developers.cam.ac.uk/uis/devops/experiments/webapp-demo/infrastructure/-/pipelines/370618

      Obviously, once an MR is open, subsequent pushes to the feature branch do trigger the extended pipeline with no issues, however, it is annoying that the pipeline doesn't run when you open the MR and it is a bit of a faff to trigger it via the UI without the nice button in the MR view.

      If we do standardise on branch jobs only, then we just need to document this quirk so that devs know that new pipelines are not triggered when opening an MR.

      Edited by Ryan Kowalewski
    • Haha, we both commented at the same time on a Sunday! Not sure what that says about us!

      I'm not suggesting not using MR pipelines, just accepting that the Auto DevOps jobs run in the branch pipeline. Our test jobs, for example, can and should still run in a MR pipeline and provide confidence that the merging into the default branch would not introduce a regression.

      I guess I just really don't like the duplicate pipelines, but I'll have to get used to it!

      If GitLab introduce a new job type, for instance "super-amazing detect security vulnerabilities through AI", then we'll just not be running it in the pipeline with the Auto DevOps jobs with no indication that we're missing functionality until someone notices and adds the appropriate rules override. That's the sort of breakage I meant.

      OK, I'll concede that new jobs would not run until we added an override to our template, which is not ideal. However, I still wouldn't consider that a break to current functionality, just us not automatically getting additional functionality (which again I do agree is not ideal).

    • I guess if I just accept running duplicate pipelines I don't have to worry about changing the terraform template at least :smile_cat:

    • Additional testing has shown some more annoyances in the merge request UI when allowing both pipeline types to run. For example, if I include the pre-commit.yml template (which runs in both branch and merge request pipelines) and the default Auto-DevOps pipelines I get two pipelines as expected (when there is an MR open).

      image

      However, what I didn't realise is that because the merge request pipeline always seems to run after the branch pipeline (and given that the merge request pipeline doesn't have any of the Auto-DevOps jobs in it) we lose visibility of the MR reports in the UI for things like tests, license compliance, container scanning etc.

      image

      If I override the pre-commit job rules to stop merge request pipelines I get a much more predictable 1 pipeline (still with the pre-commit job).

      image

      And, more importantly, the UI widgets/reports are available again.

      image

    • Please register or sign in to reply
    • Finally, I've used the *.gitlab-ci.yml template naming convention in anticipation of #52 (closed) being accepted as worthwhile.

      See #52 (comment 498328) for a way the aims of #52 (closed) could be achieved without needing this breaking change.

    • But, again to be explicit, I've no problem with using that file naming convention going forward for new files but I'd prefer that we don't have to. Otherwise we start a flame war when, e.g., JetBrains requires that they be named *.gitlab.yaml.

    • Yeah, I think I'd prefer to not have mismatched naming conventions in the repo as this might cause more annoyance when trying to include one in a pipeline (does it or doesn't it have the gitlab-ci suffix ahhhh). So, I'm happy to continue configuring my VSCode locally to lint/autocomplete.

    • Please register or sign in to reply
  • Closing this MR as this approach is currently deemed unacceptable. We will continue with Auto-DevOps jobs running only in branch-based pipelines and ensure that our custom templates work nicely with this.

  • mentioned in issue #53 (closed)

  • mentioned in issue #54 (closed)

Please register or sign in to reply
Loading