From 613acae30376b673f49031a40a91bc7f7ab18e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Alexandre=20Cunha?= Date: Fri, 4 Aug 2023 21:54:49 +0000 Subject: [PATCH 1/5] Revert "Merge branch 'revert-71b08449' into 'master'" This reverts merge request !3317 --- .gitlab-ci.yml | 25 ++++++++----------------- Dangerfile | 21 ++++++++++++++++++--- scripts/support/changelog/Dangerfile | 2 +- scripts/support/metadata/Dangerfile | 22 +++++++++++----------- scripts/support/reviewers/Dangerfile | 8 ++++---- 5 files changed, 42 insertions(+), 36 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4fe49ff8be..a914329b23 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -77,6 +77,12 @@ include: - template: Jobs/Secret-Detection.latest.gitlab-ci.yml - template: Jobs/SAST.latest.gitlab-ci.yml - template: Jobs/SAST-IaC.latest.gitlab-ci.yml + - project: 'gitlab-org/quality/pipeline-common' + file: + - '/ci/danger-review.yml' + rules: + - if: $CI_SERVER_HOST == "gitlab.com" + dependency_scanning: needs: [] before_script: [] @@ -459,23 +465,8 @@ debug_review_gke122: - if: '$PIPELINE_TYPE =~ /FEATURE_BRANCH_PIPELINE$/' danger-review: - image: registry.gitlab.com/gitlab-org/gitlab-build-images:danger - stage: prepare - cache: {} - script: - - git version - - danger_id=$(echo -n ${DANGER_GITLAB_API_TOKEN} | md5sum | awk '{print $1}' | cut -c5-10) - - | - if [ ! -v DANGER_GITLAB_API_TOKEN ] && [ "${PIPELINE_TYPE}" = "FORK_MR_PIPELINE" ]; then - echo "Danger token not configured. As this is from a fork, failing danger gracefully." - exit 202 - fi - - danger --fail-on-errors=true --verbose --danger_id=${danger_id} - allow_failure: - exit_codes: 202 - rules: - - if: '$PIPELINE_TYPE =~ /MR_PIPELINE$/' - - if: '$PIPELINE_TYPE == "DOCS_PIPELINE"' + before_script: + - bundle add gitlab-dangerfiles rubocop: image: ruby:3.0-alpine diff --git a/Dangerfile b/Dangerfile index e119761617..5026390864 100644 --- a/Dangerfile +++ b/Dangerfile @@ -1,3 +1,18 @@ -danger.import_dangerfile(path: 'scripts/support/changelog') -danger.import_dangerfile(path: 'scripts/support/metadata') -danger.import_dangerfile(path: 'scripts/support/reviewers') +require "gitlab-dangerfiles" + +# Documentation reference: https://gitlab.com/gitlab-org/ruby/gems/gitlab-dangerfiles +Gitlab::Dangerfiles.for_project(self, 'gitlab-chart') do |dangerfiles| + # Import all plugins from the gem + dangerfiles.import_plugins + + # Import a defined set of danger rules + dangerfiles.import_dangerfiles(only: %w[simple_roulette z_retry_link]) + + # These custom files have the potential to be either discarded or simplified, + # as the default rules available to us already provide much if not all of + # of our customization. Still, let's investigate this in a follow-up. + # For now, we can simply introduce the simple_roulette. + danger.import_dangerfile(path: 'scripts/support/changelog') + danger.import_dangerfile(path: 'scripts/support/metadata') + danger.import_dangerfile(path: 'scripts/support/reviewers') +end diff --git a/scripts/support/changelog/Dangerfile b/scripts/support/changelog/Dangerfile index d2d4175b40..abf4b12ee2 100644 --- a/scripts/support/changelog/Dangerfile +++ b/scripts/support/changelog/Dangerfile @@ -78,7 +78,7 @@ labels is available in #{SEE_DOC_WORKTYPE}. MSG -changelog_needed = (gitlab.mr_labels & NO_CHANGELOG_LABELS).empty? +changelog_needed = (helper.mr_labels & NO_CHANGELOG_LABELS).empty? if changelog_needed checked = 0 diff --git a/scripts/support/metadata/Dangerfile b/scripts/support/metadata/Dangerfile index 40fe9ca27b..e325a76cb6 100644 --- a/scripts/support/metadata/Dangerfile +++ b/scripts/support/metadata/Dangerfile @@ -13,43 +13,43 @@ WORKTYPE_LABELS = [ 'documentation' ].freeze -if gitlab.mr_body.size < 5 +if helper.mr_description.size < 5 fail "Please provide a proper merge request description." end -if gitlab.mr_labels.empty? +if helper.mr_labels.empty? fail "Please add labels to this merge request." end -warn "You may want to add ~group::distribution label to this MR for gitlab-insights" unless gitlab.mr_labels.include?("group::distribution") +warn "You may want to add ~group::distribution label to this MR for gitlab-insights" unless helper.mr_labels.include?("group::distribution") -unless gitlab.mr_json["assignee"] +if helper.mr_assignees.empty? warn "This merge request does not have any assignee yet. Setting an assignee clarifies who needs to take action on the merge request at any given time." end -has_milestone = !gitlab.mr_json["milestone"].nil? +has_milestone = !helper.mr_milestone.nil? unless has_milestone warn "This merge request does not refer to an existing milestone.", sticky: false end -has_pick_into_stable_label = gitlab.mr_labels.find { |label| label.start_with?('Pick into') } +has_pick_into_stable_label = helper.mr_labels.find { |label| label.start_with?('Pick into') } -if gitlab.branch_for_base != "master" && !has_pick_into_stable_label +if helper.mr_target_branch != "master" && !has_pick_into_stable_label warn "Most of the time, all merge requests should target `master`. Otherwise, please set the relevant `Pick into X.Y` label." end -has_qa_verified_label = gitlab.mr_labels.find { |label| label.start_with?('QA:verified') } -is_docs_only_branch = gitlab.branch_for_head =~ /(^docs[\/-].*|.*-docs$)/ +has_qa_verified_label = helper.mr_labels.find { |label| label.start_with?('QA:verified') } +is_docs_only_branch = helper.mr_source_branch =~ /(^docs[\/-].*|.*-docs$)/ unless has_qa_verified_label || is_docs_only_branch warn "Please check the QA job and compare with builds on master. If no new failures are reported in QA job, add 'QA:verified' label." end -if (gitlab.mr_labels & WORKTYPE_LABELS).empty? +if (helper.mr_labels & WORKTYPE_LABELS).empty? warn "This merge request is missing any [Work Type Classification labels](https://about.gitlab.com/handbook/engineering/metrics/#work-type-classification)." end -unless gitlab.mr_json["force_remove_source_branch"] +if helper.ci? && !gitlab.mr_json["force_remove_source_branch"] warn "If the source branch can be safely removed after this merge request is accepted, please consider setting the '[delete source branch when merge request is accepted](https://docs.gitlab.com/ee/user/project/merge_requests/getting_started.html#deleting-the-source-branch)' option." end diff --git a/scripts/support/reviewers/Dangerfile b/scripts/support/reviewers/Dangerfile index debd97807a..37ea097486 100644 --- a/scripts/support/reviewers/Dangerfile +++ b/scripts/support/reviewers/Dangerfile @@ -1,13 +1,13 @@ # frozen_string_literal: true MESSAGE = <` to request the first review to someone. It's recommended that you pick the one suggested by the reviewre roulette. But you can ignore it and assign it to someone else if you see fit. -Merge requests are handled according to the workflow documented in our [handbook](https://about.gitlab.com/handbook/engineering/development/enablement/systems/distribution/merge_requests.html) and should receive a response within the limit documented in our [First-response SLO](https://about.gitlab.com/handbook/engineering/workflow/code-review/#first-response-slo). +Merge requests are handled according to the workflow documented in our [handbook](https://about.gitlab.com/handbook/engineering/development/enablement/systems/distribution/merge_requests.html#experimental-workflow) and should receive a response within the limit documented in our [First-response SLO](https://about.gitlab.com/handbook/engineering/workflow/code-review/#first-response-slo). -If you don't receive a response, please mention `@gitlab-org\/distribution`, or one of our [Project Maintainers](https://about.gitlab.com/handbook/engineering/projects/#gitlab-chart) +If you don't receive a response from the reviewer within the SLO, please mention `@gitlab-org\/distribution`, or one of our [Project Maintainers](https://about.gitlab.com/handbook/engineering/projects/#gitlab-chart) REV_EOF # Print maintainers message -- GitLab From 612e179b34abb94ff4eddaf7580d7c85d1a33d31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Fri, 4 Aug 2023 18:57:24 -0300 Subject: [PATCH 2/5] Define danger-review before including danger-review To not duplicate ourselves, we want to include the danger-review definition from gitlab-org/quality/pipeline-common. But this project only exists in gitlab.com. So, we can't rely on the `script` tag defined there to be the sole definition for the `script` tag inside the danger-review job. This is because, when we trigger a pipeline in dev.gitlab.org, the aforementioned template doesn't exist and the danger-review job would endup with no script tag, which breaks the pipeline parsing. By defining the job before including the template, we can create a `no-op` script tag just so that the pipeline is correctly parsed in dev.gitlab.org. But the job needs to be defined before including the template because, for the cases where we do include the template, we want the template script definition to override the `no-op` job definition. --- .gitlab-ci.yml | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a914329b23..57dc6d1cc9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -71,6 +71,16 @@ stages: - cleanup - report +# This job MUST be defined before including /ci/danger-review.yml below +# because we MUST override the `script` key in the correct order +danger-review: + # This script only exists so that this yaml file is correctly parsed on dev.gitlab.org + # where /ci/danger-review.yml won't be included. In gitlab.com this will be included + # and the script will be overridden. + script: echo "no-op" + rules: + - if: $CI_SERVER_HOST == "gitlab.com" + include: - local: '/.gitlab/ci/rules.gitlab-ci.yml' - template: Jobs/Dependency-Scanning.latest.gitlab-ci.yml @@ -464,10 +474,6 @@ debug_review_gke122: - if: '$PIPELINE_TYPE =~ /STABLE_BRANCH_PIPELINE$/' - if: '$PIPELINE_TYPE =~ /FEATURE_BRANCH_PIPELINE$/' -danger-review: - before_script: - - bundle add gitlab-dangerfiles - rubocop: image: ruby:3.0-alpine stage: prepare -- GitLab From 061e2d167532d9b168a201057c3b78f87ccb9a3a Mon Sep 17 00:00:00 2001 From: Tiger Watson Date: Tue, 8 Aug 2023 19:25:19 +0000 Subject: [PATCH 3/5] Fix typo in Dangerfile message --- scripts/support/reviewers/Dangerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/support/reviewers/Dangerfile b/scripts/support/reviewers/Dangerfile index 37ea097486..36bcfc04f4 100644 --- a/scripts/support/reviewers/Dangerfile +++ b/scripts/support/reviewers/Dangerfile @@ -3,7 +3,7 @@ MESSAGE = <` to request the first review to someone. It's recommended that you pick the one suggested by the reviewre roulette. But you can ignore it and assign it to someone else if you see fit. +Once your MR is ready for review you can comment `@gitlab-bot ready <@user>` to request the first review to someone. It's recommended that you pick the one suggested by the reviewer roulette. But you can ignore it and assign it to someone else if you see fit. Merge requests are handled according to the workflow documented in our [handbook](https://about.gitlab.com/handbook/engineering/development/enablement/systems/distribution/merge_requests.html#experimental-workflow) and should receive a response within the limit documented in our [First-response SLO](https://about.gitlab.com/handbook/engineering/workflow/code-review/#first-response-slo). -- GitLab From 17e732d0aaab21ccea74055f7f994b057144cd5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Wed, 9 Aug 2023 18:16:15 -0300 Subject: [PATCH 4/5] Update MR template with roulette instructions --- .gitlab/merge_request_templates/Default.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab/merge_request_templates/Default.md b/.gitlab/merge_request_templates/Default.md index 3dc7760a70..97dbc4b47b 100644 --- a/.gitlab/merge_request_templates/Default.md +++ b/.gitlab/merge_request_templates/Default.md @@ -20,7 +20,7 @@ For anything in this list which will not be completed, please provide a reason i - [ ] Merge Request Title and Description are up to date, accurate, and descriptive - [ ] MR targeting the appropriate branch - [ ] MR has a green pipeline on GitLab.com -- [ ] When ready for review, MR is labeled "~workflow::ready for review" per the [Distribution MR workflow](https://about.gitlab.com/handbook/engineering/development/enablement/systems/distribution/merge_requests.html) +- [ ] When ready for review, follow the instructions in the "Reviewer Roulette" section of the Danger Bot MR comment, as per the [Distribution experimental MR workflow](https://about.gitlab.com/handbook/engineering/development/enablement/systems/distribution/merge_requests.html) ### Expected (please provide an explanation if not completing) - [ ] Test plan indicating conditions for success has been posted and passes -- GitLab From 500279def5412677e18af8f91ab3ef8c8e0aec0a Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Mon, 14 Aug 2023 22:07:11 +0000 Subject: [PATCH 5/5] Fix markdown link --- scripts/support/reviewers/Dangerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/support/reviewers/Dangerfile b/scripts/support/reviewers/Dangerfile index 36bcfc04f4..6233c441b2 100644 --- a/scripts/support/reviewers/Dangerfile +++ b/scripts/support/reviewers/Dangerfile @@ -5,7 +5,7 @@ If this is a wider community contribution, the ~"Community contribution" label s Once your MR is ready for review you can comment `@gitlab-bot ready <@user>` to request the first review to someone. It's recommended that you pick the one suggested by the reviewer roulette. But you can ignore it and assign it to someone else if you see fit. -Merge requests are handled according to the workflow documented in our [handbook](https://about.gitlab.com/handbook/engineering/development/enablement/systems/distribution/merge_requests.html#experimental-workflow) and should receive a response within the limit documented in our [First-response SLO](https://about.gitlab.com/handbook/engineering/workflow/code-review/#first-response-slo). +Merge requests are handled according to the workflow documented in our [handbook](https://about.gitlab.com/handbook/engineering/development/enablement/systems/distribution/merge_requests.html#workflow) and should receive a response within the limit documented in our [First-response SLO](https://about.gitlab.com/handbook/engineering/workflow/code-review/#first-response-slo). If you don't receive a response from the reviewer within the SLO, please mention `@gitlab-org\/distribution`, or one of our [Project Maintainers](https://about.gitlab.com/handbook/engineering/projects/#gitlab-chart) REV_EOF -- GitLab