Skip to content

Add danger for ci rule dependency validation

Jennifer Li requested to merge jennli-add-danger-for-ci-rule-dependency into master

What does this MR do and why?

Re: #34040

Add CI rule dependency validation in pipeline Dangerfile

How will this work:

  • When I added a new rule condition to job1, and if job1 needs one the jobs listed in NEEDED_JOB_NAMES, danger will flag this job with the added rule condition, warning reviewer/author about this job dependency, and will show in a collapsed section what the needed job's current rule looks like. This can simplify the code review because we can just compare the parsed rules side by side.

Example (copied from !157109 (closed)):

This MR adds new rules to the following dependent jobs for setup-test-env:

db:rollback single-db-ci-connection:

- if: $THIS_VARIABLE_DOES_NOT_EXIST == "true"

db:migrate:reset single-db-ci-connection:

- if: $THIS_VARIABLE_DOES_NOT_EXIST == "true"

Please ensure the changes are included in the rules for setup-test-env to avoid yaml syntax error!

Click to expand rules for setup-test-env to confirm if the new conditions are present
- if: $ENABLE_RSPEC == "true"
- if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH || $CI_COMMIT_REF_NAME =~ /^[\d-]+-stable(-ee)?$/
    || $CI_COMMIT_REF_NAME =~ /^\d+-\d+-auto-deploy-\d+$/ || $CI_COMMIT_REF_NAME =~
    /^security\// || $CI_COMMIT_REF_NAME =~ /^ruby\d+(_\d)*$/ || ($CI_MERGE_REQUEST_EVENT_TYPE
    == "merged_result" || $CI_MERGE_REQUEST_EVENT_TYPE == "detached") || $CI_COMMIT_TAG
    || $FORCE_GITLAB_CI
  changes:
  - "{package.json,yarn.lock}"
  - ".browserslistrc"
  - babel.config.js
  - jest.config.{base,integration,unit}.js
  - ".stylelintrc"
  - Dockerfile.assets
  - vendor/assets/**/*
  - ".{eslintrc.yml,eslintignore,gitattributes,nvmrc,prettierrc,stylelintrc,yamllint}"
  - "*_VERSION"
  - "{,jh/}Gemfile{,.lock}"
  - keeps/**/*
  - Rakefile
  - tests.yml
  - config.ru
  - "{,ee/,jh/}{app,bin,config,db,elastic,generator_templates,gems,haml_lint,lib,locale,public,scripts,sidekiq_cluster,storybook,symbol,vendor}/**/*"
  - doc/api/graphql/reference/*
  - "{,jh/}.gitlab-ci.yml"
  - "{,jh/}.gitlab/ci/**/*"
  - data/whats_new/*.yml
  - doc/index.md
  - Dangerfile
  - danger/**/*
  - "{,ee/,jh/}fixtures/**/*"
  - "{,ee/,jh/}rubocop/**/*"
  - "{,ee/,jh/}spec/**/*"
  - "{,spec/}tooling/**/*"
  - ".dockerignore"
  - "{,jh/}qa/**/*"
  - ".gitlab/ci/workhorse.gitlab-ci.yml"
  - GITLAB_WORKHORSE_VERSION
  - workhorse/**/*
  - scripts/gitaly-test-build
  - scripts/gitaly-test-spawn
  - spec/support/gitlab-git-test.git/**/*
  - spec/support/helpers/gitaly_setup.rb
  - GITALY_SERVER_VERSION
  - lib/gitlab/setup_helper.rb
  - scripts/lint_templates_bash.rb
  - lib/gitlab/ci/templates/**/*.gitlab-ci.yml
  - glfm_specification/**/*
- if: ($CI_MERGE_REQUEST_EVENT_TYPE == "merged_result" || $CI_MERGE_REQUEST_EVENT_TYPE
    == "detached") && $CI_MERGE_REQUEST_LABELS =~ /pipeline:run-all-rspec/

Iterations needed for

  • The danger output could become very big if we are adding rules for a lot of jobs, so we need to account for such MRs with massive CI rule updates.
  • If the branch is very old and is missing the latest CI config changes, it will also report warnings. We should improve the message to suggest rebase if they think the rule diff is due to this reason.
  • Removing rule conditions from a needed job could also result in yaml syntax error. I'm not handling this use case in this MR yet.
  • This message might be forgotten as it doesn't block MR. Pipeline maintainers must know to check the danger warning when approving such MR.
  • Once the missing rule is added to the needed job, the warning message could still be here, if the rule looks a little different. For example:
- rule-for-job1:
  - <<: *if-merge-request-targeting-stable-branch
      changes: *setup-test-env-patterns

- rule-for-needed_job:
  - <<: *if-merge-request-targeting-all-branches
      changes: *setup-test-env-patterns

In the example above, condition for rule-for-needed_job is not exactly identical to the rule we are adding to job1, but it is a superset rule for the scenario for job1. In this case the warning will still be on the MR, but it doesn't require any fix. I can't really think of any ways to improve for this yet. I believe the best practices is to always ensure dependent jobs have identical rule definitions as their needed jobs, but this is not always true in our repo right now.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Edited by Jennifer Li

Merge request reports