Merge branch...

Merge branch '299463-make-danger-warn-when-a-feature-flag-is-removed-and-title-doesn-t-include-run-all-rspec-run' into 'master'

Make Danger warn when a feature flag is removed and title doesn't include required flags [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!52814

.gitlab/ci/rails.gitlab-ci.yml

@@ -502,7 +502,6 @@ rspec:feature-flags:
     - .coverage-base
     - .rails:rules:rspec-feature-flags
   stage: post-test
-  allow_failure: true
   # We cannot use needs since it would mean needing 84 jobs (since most are parallelized)
   # so we use `dependencies` here.
   dependencies:
@@ -522,7 +521,11 @@ rspec:feature-flags:
     - memory-on-boot
   script:
     - run_timed_command "bundle install --jobs=$(nproc) --path=vendor --retry=3 --quiet --without default development test production puma unicorn kerberos metrics omnibus ed25519"
-    - 'run_timed_command "bundle exec scripts/used-feature-flags" || (scripts/slack master-broken "☠️ \`${CI_JOB_NAME}\` failed! ☠️ See ${CI_JOB_URL}" ci_failing "GitLab Bot" && exit 1)'
+    - if [ "$CI_COMMIT_BRANCH" == "$CI_DEFAULT_BRANCH" ]; then
+        run_timed_command "bundle exec scripts/used-feature-flags" || (scripts/slack master-broken "☠️ \`${CI_JOB_NAME}\` failed! ☠️ See ${CI_JOB_URL}" ci_failing "GitLab Bot" && exit 1);
+      else
+        run_timed_command "bundle exec scripts/used-feature-flags";
+      fi
 
 # EE/FOSS: default refs (MRs, master, schedules) jobs #
 #######################################################

.gitlab/ci/rules.gitlab-ci.yml

@@ -879,6 +879,7 @@
     - <<: *if-not-ee
       when: never
     - <<: *if-master-schedule-2-hourly
+      allow_failure: true
     - <<: *if-merge-request-title-run-all-rspec
 
 .rails:rules:master-schedule-nightly--code-backstage:

danger/feature_flag/Dangerfile

@@ -52,6 +52,23 @@ def message_for_feature_flag_with_group!(feature_flag:, mr_group_label:)
   end
 end
 
-feature_flag.feature_flag_files.each do |feature_flag|
+feature_flag.feature_flag_files(change_type: :added).each do |feature_flag|
   check_feature_flag_yaml(feature_flag)
 end
+
+if feature_flag.feature_flag_files(change_type: :added).any? ||
+  feature_flag.feature_flag_files(change_type: :deleted).any?
+  new_mr_title = helper.mr_title.dup
+  new_mr_title << ' [RUN ALL RSPEC]' unless helper.run_all_rspec_mr?
+  new_mr_title << ' [RUN AS-IF-FOSS]' unless helper.run_as_if_foss_mr?
+
+  if new_mr_title != helper.mr_title
+    gitlab.api.update_merge_request(
+      gitlab.mr_json['project_id'],
+      gitlab.mr_json['iid'],
+      title: new_mr_title
+    )
+    gitlab.api.post("/projects/#{gitlab.mr_json['project_id']}/merge_requests/#{gitlab.mr_json['iid']}/pipelines")
+    message %(You're adding or removing a feature flag, and your MR title didn't include `[RUN ALL RSPEC] [RUN AS-IF-FOSS]`, so we've updated it and started a new MR pipeline to ensure everything is covered.)
+  end
+end

spec/tooling/danger/feature_flag_spec.rb

@@ -8,7 +8,9 @@ RSpec.describe Tooling::Danger::FeatureFlag do
   include DangerSpecHelper
 
   let(:added_files) { nil }
-  let(:fake_git) { double('fake-git', added_files: added_files) }
+  let(:modified_files) { nil }
+  let(:deleted_files) { nil }
+  let(:fake_git) { double('fake-git', added_files: added_files, modified_files: modified_files, deleted_files: deleted_files) }
 
   let(:mr_labels) { nil }
   let(:mr_json) { nil }
@@ -24,29 +26,72 @@ RSpec.describe Tooling::Danger::FeatureFlag do
   subject(:feature_flag) { fake_danger.new(git: fake_git, gitlab: fake_gitlab, helper: fake_helper) }
 
   describe '#feature_flag_files' do
-    context 'added files contain several feature flags' do
-      let(:added_files) do
-        [
-          'config/feature_flags/development/entry.yml',
-          'ee/config/feature_flags/ops/entry.yml'
-        ]
+    let(:feature_flag_files) do
+      [
+        'config/feature_flags/development/entry.yml',
+        'ee/config/feature_flags/ops/entry.yml'
+      ]
+    end
+
+    let(:other_files) do
+      [
+        'app/models/model.rb',
+        'app/assets/javascripts/file.js'
+      ]
+    end
+
+    shared_examples 'an array of Found objects' do |change_type|
+      it 'returns an array of Found objects' do
+        expect(feature_flag.feature_flag_files(change_type: change_type)).to contain_exactly(an_instance_of(described_class::Found), an_instance_of(described_class::Found))
+        expect(feature_flag.feature_flag_files(change_type: change_type).map(&:path)).to eq(feature_flag_files)
       end
+    end
 
+    shared_examples 'an empty array' do |change_type|
       it 'returns an array of Found objects' do
-        expect(feature_flag.feature_flag_files).to contain_exactly(an_instance_of(described_class::Found), an_instance_of(described_class::Found))
+        expect(feature_flag.feature_flag_files(change_type: change_type)).to be_empty
+      end
+    end
+
+    describe 'retrieves added feature flag files' do
+      context 'with added added feature flag files' do
+        let(:added_files) { feature_flag_files }
+
+        include_examples 'an array of Found objects', :added
+      end
+
+      context 'without added added feature flag files' do
+        let(:added_files) { other_files }
+
+        include_examples 'an empty array', :added
       end
     end
 
-    context 'added files do not contain a feature_flag' do
-      let(:added_files) do
-        [
-          'app/models/model.rb',
-          'app/assets/javascripts/file.js'
-        ]
+    describe 'retrieves modified feature flag files' do
+      context 'with modified modified feature flag files' do
+        let(:modified_files) { feature_flag_files }
+
+        include_examples 'an array of Found objects', :modified
+      end
+
+      context 'without modified modified feature flag files' do
+        let(:modified_files) { other_files }
+
+        include_examples 'an empty array', :modified
       end
+    end
+
+    describe 'retrieves deleted feature flag files' do
+      context 'with deleted deleted feature flag files' do
+        let(:deleted_files) { feature_flag_files }
+
+        include_examples 'an array of Found objects', :deleted
+      end
+
+      context 'without deleted deleted feature flag files' do
+        let(:deleted_files) { other_files }
 
-      it 'returns the feature flag file path' do
-        expect(feature_flag.feature_flag_files).to be_empty
+        include_examples 'an empty array', :deleted
       end
     end
   end

spec/tooling/danger/helper_spec.rb

@@ -402,13 +402,55 @@ RSpec.describe Tooling::Danger::Helper do
     end
   end
 
-  describe '#security_mr?' do
-    it 'returns false when `gitlab_helper` is unavailable' do
+  describe '#mr_title' do
+    it 'returns "" when `gitlab_helper` is unavailable' do
       expect(helper).to receive(:gitlab_helper).and_return(nil)
 
-      expect(helper).not_to be_security_mr
+      expect(helper.mr_title).to eq('')
     end
 
+    it 'returns the MR title when `gitlab_helper` is available' do
+      mr_title = 'My MR title'
+      expect(fake_gitlab).to receive(:mr_json)
+        .and_return('title' => mr_title)
+
+      expect(helper.mr_title).to eq(mr_title)
+    end
+  end
+
+  describe '#mr_web_url' do
+    it 'returns "" when `gitlab_helper` is unavailable' do
+      expect(helper).to receive(:gitlab_helper).and_return(nil)
+
+      expect(helper.mr_web_url).to eq('')
+    end
+
+    it 'returns the MR web_url when `gitlab_helper` is available' do
+      mr_web_url = 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1'
+      expect(fake_gitlab).to receive(:mr_json)
+        .and_return('web_url' => mr_web_url)
+
+      expect(helper.mr_web_url).to eq(mr_web_url)
+    end
+  end
+
+  describe '#mr_target_branch' do
+    it 'returns "" when `gitlab_helper` is unavailable' do
+      expect(helper).to receive(:gitlab_helper).and_return(nil)
+
+      expect(helper.mr_target_branch).to eq('')
+    end
+
+    it 'returns the MR web_url when `gitlab_helper` is available' do
+      mr_target_branch = 'main'
+      expect(fake_gitlab).to receive(:mr_json)
+        .and_return('target_branch' => mr_target_branch)
+
+      expect(helper.mr_target_branch).to eq(mr_target_branch)
+    end
+  end
+
+  describe '#security_mr?' do
     it 'returns false when on a normal merge request' do
       expect(fake_gitlab).to receive(:mr_json)
         .and_return('web_url' => 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1')
@@ -425,12 +467,6 @@ RSpec.describe Tooling::Danger::Helper do
   end
 
   describe '#draft_mr?' do
-    it 'returns false when `gitlab_helper` is unavailable' do
-      expect(helper).to receive(:gitlab_helper).and_return(nil)
-
-      expect(helper).not_to be_draft_mr
-    end
-
     it 'returns true for a draft MR' do
       expect(fake_gitlab).to receive(:mr_json)
         .and_return('title' => 'Draft: My MR title')
@@ -447,12 +483,6 @@ RSpec.describe Tooling::Danger::Helper do
   end
 
   describe '#cherry_pick_mr?' do
-    it 'returns false when `gitlab_helper` is unavailable' do
-      expect(helper).to receive(:gitlab_helper).and_return(nil)
-
-      expect(helper).not_to be_cherry_pick_mr
-    end
-
     context 'when MR title does not mention a cherry-pick' do
       it 'returns false' do
         expect(fake_gitlab).to receive(:mr_json)
@@ -478,6 +508,46 @@ RSpec.describe Tooling::Danger::Helper do
     end
   end
 
+  describe '#run_all_rspec_mr?' do
+    context 'when MR title does not mention RUN ALL RSPEC' do
+      it 'returns false' do
+        expect(fake_gitlab).to receive(:mr_json)
+          .and_return('title' => 'Add feature xyz')
+
+        expect(helper).not_to be_run_all_rspec_mr
+      end
+    end
+
+    context 'when MR title mentions RUN ALL RSPEC' do
+      it 'returns true' do
+        expect(fake_gitlab).to receive(:mr_json)
+          .and_return('title' => 'Add feature xyz RUN ALL RSPEC')
+
+        expect(helper).to be_run_all_rspec_mr
+      end
+    end
+  end
+
+  describe '#run_as_if_foss_mr?' do
+    context 'when MR title does not mention RUN AS-IF-FOSS' do
+      it 'returns false' do
+        expect(fake_gitlab).to receive(:mr_json)
+          .and_return('title' => 'Add feature xyz')
+
+        expect(helper).not_to be_run_as_if_foss_mr
+      end
+    end
+
+    context 'when MR title mentions RUN AS-IF-FOSS' do
+      it 'returns true' do
+        expect(fake_gitlab).to receive(:mr_json)
+          .and_return('title' => 'Add feature xyz RUN AS-IF-FOSS')
+
+        expect(helper).to be_run_as_if_foss_mr
+      end
+    end
+  end
+
   describe '#stable_branch?' do
     it 'returns false when `gitlab_helper` is unavailable' do
       expect(helper).to receive(:gitlab_helper).and_return(nil)

spec/tooling/danger/title_linting_spec.rb

@@ -52,4 +52,40 @@ RSpec.describe Tooling::Danger::TitleLinting do
       expect(described_class.has_draft_flag?('My MR title')).to be false
     end
   end
+
+  describe '#has_cherry_pick_flag?' do
+    [
+      'Cherry Pick !1234',
+      'cherry-pick !1234',
+      'CherryPick !1234'
+    ].each do |mr_title|
+      it 'returns true for cherry-pick title' do
+        expect(described_class.has_cherry_pick_flag?(mr_title)).to be true
+      end
+    end
+
+    it 'returns false for non cherry-pick title' do
+      expect(described_class.has_cherry_pick_flag?('My MR title')).to be false
+    end
+  end
+
+  describe '#has_run_all_rspec_flag?' do
+    it 'returns true for a title that includes RUN ALL RSPEC' do
+      expect(described_class.has_run_all_rspec_flag?('My MR title RUN ALL RSPEC')).to be true
+    end
+
+    it 'returns true for a title that does not include RUN ALL RSPEC' do
+      expect(described_class.has_run_all_rspec_flag?('My MR title')).to be false
+    end
+  end
+
+  describe '#has_run_as_if_foss_flag?' do
+    it 'returns true for a title that includes RUN AS-IF-FOSS' do
+      expect(described_class.has_run_as_if_foss_flag?('My MR title RUN AS-IF-FOSS')).to be true
+    end
+
+    it 'returns true for a title that does not include RUN AS-IF-FOSS' do
+      expect(described_class.has_run_as_if_foss_flag?('My MR title')).to be false
+    end
+  end
 end

tooling/danger/feature_flag.rb

@@ -5,8 +5,13 @@ require 'yaml'
 module Tooling
   module Danger
     module FeatureFlag
-      def feature_flag_files
-        @feature_flag_files ||= git.added_files.select { |path| path =~ %r{\A(ee/)?config/feature_flags/} }.map { |path| Found.new(path) }
+      # `change_type` can be:
+      #   - :added
+      #   - :modified
+      #   - :deleted
+      def feature_flag_files(change_type:)
+        files = git.public_send("#{change_type}_files") # rubocop:disable GitlabSecurity/PublicSend
+        files.select { |path| path =~ %r{\A(ee/)?config/feature_flags/} }.map { |path| Found.new(path) }
       end
 
       class Found

tooling/danger/helper.rb

@@ -215,28 +215,46 @@ module Tooling
         usernames.map { |u| Tooling::Danger::Teammate.new('username' => u) }
       end
 
-      def draft_mr?
-        return false unless gitlab_helper
+      def mr_title
+        return '' unless gitlab_helper
 
-        TitleLinting.has_draft_flag?(gitlab_helper.mr_json['title'])
+        gitlab_helper.mr_json['title']
       end
 
-      def security_mr?
-        return false unless gitlab_helper
+      def mr_web_url
+        return '' unless gitlab_helper
+
+        gitlab_helper.mr_json['web_url']
+      end
+
+      def mr_target_branch
+        return '' unless gitlab_helper
 
-        gitlab_helper.mr_json['web_url'].include?('/gitlab-org/security/')
+        gitlab_helper.mr_json['target_branch']
+      end
+
+      def draft_mr?
+        TitleLinting.has_draft_flag?(mr_title)
+      end
+
+      def security_mr?
+        mr_web_url.include?('/gitlab-org/security/')
       end
 
       def cherry_pick_mr?
-        return false unless gitlab_helper
+        TitleLinting.has_cherry_pick_flag?(mr_title)
+      end
 
-        /cherry[\s-]*pick/i.match?(gitlab_helper.mr_json['title'])
+      def run_all_rspec_mr?
+        TitleLinting.has_run_all_rspec_flag?(mr_title)
       end
 
-      def stable_branch?
-        return false unless gitlab_helper
+      def run_as_if_foss_mr?
+        TitleLinting.has_run_as_if_foss_flag?(mr_title)
+      end
 
-        /\A\d+-\d+-stable-ee/i.match?(gitlab_helper.mr_json['target_branch'])
+      def stable_branch?
+        /\A\d+-\d+-stable-ee/i.match?(mr_target_branch)
       end
 
       def mr_has_labels?(*labels)

tooling/danger/title_linting.rb

@@ -4,6 +4,9 @@ module Tooling
   module Danger
     module TitleLinting
       DRAFT_REGEX = /\A*#{Regexp.union(/(?i)(\[WIP\]\s*|WIP:\s*|WIP$)/, /(?i)(\[draft\]|\(draft\)|draft:|draft\s\-\s|draft$)/)}+\s*/i.freeze
+      CHERRY_PICK_REGEX = /cherry[\s-]*pick/i.freeze
+      RUN_ALL_RSPEC_REGEX = /RUN ALL RSPEC/i.freeze
+      RUN_AS_IF_FOSS_REGEX = /RUN AS-IF-FOSS/i.freeze
 
       module_function
 
@@ -18,6 +21,18 @@ module Tooling
       def has_draft_flag?(title)
         DRAFT_REGEX.match?(title)
       end
+
+      def has_cherry_pick_flag?(title)
+        CHERRY_PICK_REGEX.match?(title)
+      end
+
+      def has_run_all_rspec_flag?(title)
+        RUN_ALL_RSPEC_REGEX.match?(title)
+      end
+
+      def has_run_as_if_foss_flag?(title)
+        RUN_AS_IF_FOSS_REGEX.match?(title)
+      end
     end
   end
 end