Merge branch 'make-sure-feature-flag-group-match-group-label' into 'master'

New Danger check to ensure a FF group is the same as the MR group label

See merge request gitlab-org/gitlab!52239

Dangerfile

@@ -3,10 +3,7 @@
 require_relative 'tooling/gitlab_danger'
 require_relative 'tooling/danger/request_helper'
 
-danger.import_plugin('danger/plugins/helper.rb')
-danger.import_plugin('danger/plugins/roulette.rb')
-danger.import_plugin('danger/plugins/changelog.rb')
-danger.import_plugin('danger/plugins/sidekiq_queues.rb')
+Dir["danger/plugins/*.rb"].sort.each { |f| danger.import_plugin(f) }
 
 return if helper.release_automation?
 

danger/changelog/Dangerfile

@@ -35,7 +35,7 @@ def check_changelog_yaml(path)
   elsif yaml["merge_request"] != gitlab.mr_json["iid"] && !cherry_pick_against_stable_branch
     fail "Merge request ID was not set to #{gitlab.mr_json["iid"]}! #{SEE_DOC}"
   end
-rescue Psych::SyntaxError, Psych::DisallowedClass, Psych::BadAlias
+rescue Psych::Exception
   # YAML could not be parsed, fail the build.
   fail "#{gitlab.html_link(path)} isn't valid YAML! #{SEE_DOC}"
 rescue StandardError => e

danger/feature_flag/Dangerfile

+# frozen_string_literal: true
+# rubocop:disable Style/SignalException
+
+SEE_DOC = "See the [feature flag documentation](https://docs.gitlab.com/ee/development/feature_flags/development.html#feature-flag-definition-and-validation)."
+
+SUGGEST_MR_COMMENT = <<~SUGGEST_COMMENT
+```suggestion
+group: "%<group>s"
+```
+
+#{SEE_DOC}
+SUGGEST_COMMENT
+
+def check_feature_flag_yaml(feature_flag)
+  mr_group_label = helper.group_label(gitlab.mr_labels)
+
+  if feature_flag.group.nil?
+    message_for_feature_flag_missing_group!(feature_flag: feature_flag, mr_group_label: mr_group_label)
+  else
+    message_for_feature_flag_with_group!(feature_flag: feature_flag, mr_group_label: mr_group_label)
+  end
+rescue Psych::Exception
+  # YAML could not be parsed, fail the build.
+  fail "#{gitlab.html_link(feature_flag.path)} isn't valid YAML! #{SEE_DOC}"
+rescue StandardError => e
+  warn "There was a problem trying to check the Feature Flag file. Exception: #{e.class.name} - #{e.message}"
+end
+
+def message_for_feature_flag_missing_group!(feature_flag:, mr_group_label:)
+  if mr_group_label.nil?
+    warn "Consider setting `group` in #{gitlab.html_link(feature_flag.path)}. #{SEE_DOC}"
+  else
+    mr_line = feature_flag.raw.lines.find_index("group:\n")
+
+    if mr_line
+      markdown(format(SUGGEST_MR_COMMENT, group: mr_group_label), file: feature_flag.path, line: mr_line.succ)
+    else
+      warn %(Consider setting `group: "#{mr_group_label}"` in #{gitlab.html_link(feature_flag.path)}. #{SEE_DOC})
+    end
+  end
+end
+
+def message_for_feature_flag_with_group!(feature_flag:, mr_group_label:)
+  return if feature_flag.group_match_mr_label?(mr_group_label)
+
+  if mr_group_label.nil?
+    gitlab.api.update_merge_request(gitlab.mr_json['project_id'],
+                                    gitlab.mr_json['iid'],
+                                    add_labels: feature_flag.group)
+  else
+    fail %(`group` is set to ~"#{feature_flag.group}" in #{gitlab.html_link(feature_flag.path)}, which does not match ~"#{mr_group_label}" set on the MR!)
+  end
+end
+
+feature_flag.feature_flag_files.each do |feature_flag|
+  check_feature_flag_yaml(feature_flag)
+end

danger/plugins/feature_flag.rb

+# frozen_string_literal: true
+
+require_relative '../../tooling/danger/feature_flag'
+
+module Danger
+  class FeatureFlag < Plugin
+    # Put the helper code somewhere it can be tested
+    include Tooling::Danger::FeatureFlag
+  end
+end

spec/tooling/danger/feature_flag_spec.rb

+# frozen_string_literal: true
+
+require_relative 'danger_spec_helper'
+
+require_relative '../../../tooling/danger/feature_flag'
+
+RSpec.describe Tooling::Danger::FeatureFlag do
+  include DangerSpecHelper
+
+  let(:added_files) { nil }
+  let(:fake_git) { double('fake-git', added_files: added_files) }
+
+  let(:mr_labels) { nil }
+  let(:mr_json) { nil }
+  let(:fake_gitlab) { double('fake-gitlab', mr_labels: mr_labels, mr_json: mr_json) }
+
+  let(:changes_by_category) { nil }
+  let(:sanitize_mr_title) { nil }
+  let(:ee?) { false }
+  let(:fake_helper) { double('fake-helper', changes_by_category: changes_by_category, sanitize_mr_title: sanitize_mr_title, ee?: ee?) }
+
+  let(:fake_danger) { new_fake_danger.include(described_class) }
+
+  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'
+        ]
+      end
+
+      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))
+      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'
+        ]
+      end
+
+      it 'returns the feature flag file path' do
+        expect(feature_flag.feature_flag_files).to be_empty
+      end
+    end
+  end
+
+  describe described_class::Found do
+    let(:feature_flag_path) { 'config/feature_flags/development/entry.yml' }
+    let(:group) { 'group::source code' }
+    let(:raw_yaml) do
+      YAML.dump('group' => group)
+    end
+
+    subject(:found) { described_class.new(feature_flag_path) }
+
+    before do
+      allow(File).to receive(:read).and_call_original
+      expect(File).to receive(:read).with(feature_flag_path).and_return(raw_yaml)
+    end
+
+    describe '#raw' do
+      it 'returns the raw YAML' do
+        expect(found.raw).to eq(raw_yaml)
+      end
+    end
+
+    describe '#group' do
+      it 'returns the group found in the YAML' do
+        expect(found.group).to eq(group)
+      end
+    end
+
+    describe '#group_match_mr_label?' do
+      subject(:result) { found.group_match_mr_label?(mr_group_label) }
+
+      context 'when MR labels match FF group' do
+        let(:mr_group_label) { 'group::source code' }
+
+        specify { expect(result).to eq(true) }
+      end
+
+      context 'when MR labels does not match FF group' do
+        let(:mr_group_label) { 'group::access' }
+
+        specify { expect(result).to eq(false) }
+      end
+
+      context 'when group is nil' do
+        let(:group) { nil }
+
+        context 'and MR has no group label' do
+          let(:mr_group_label) { nil }
+
+          specify { expect(result).to eq(true) }
+        end
+
+        context 'and MR has a group label' do
+          let(:mr_group_label) { 'group::source code' }
+
+          specify { expect(result).to eq(false) }
+        end
+      end
+    end
+  end
+end

spec/tooling/danger/helper_spec.rb

@@ -599,4 +599,14 @@ RSpec.describe Tooling::Danger::Helper do
       end
     end
   end
+
+  describe '#group_label' do
+    it 'returns nil when no group label is present' do
+      expect(helper.group_label(%w[foo bar])).to be_nil
+    end
+
+    it 'returns the group label when a group label is present' do
+      expect(helper.group_label(['foo', 'group::source code', 'bar'])).to eq('group::source code')
+    end
+  end
 end

tooling/danger/feature_flag.rb

+# frozen_string_literal: true
+
+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) }
+      end
+
+      class Found
+        attr_reader :path
+
+        def initialize(path)
+          @path = path
+        end
+
+        def raw
+          @raw ||= File.read(path)
+        end
+
+        def group
+          @group ||= yaml['group']
+        end
+
+        def group_match_mr_label?(mr_group_label)
+          mr_group_label == group
+        end
+
+        private
+
+        def yaml
+          @yaml ||= YAML.safe_load(raw)
+        end
+      end
+    end
+  end
+end

tooling/danger/helper.rb

@@ -260,13 +260,17 @@ module Tooling
         all_changed_files.grep(regex)
       end
 
-      def has_database_scoped_labels?(current_mr_labels)
-        current_mr_labels.any? { |label| label.start_with?('database::') }
+      def has_database_scoped_labels?(labels)
+        labels.any? { |label| label.start_with?('database::') }
       end
 
       def has_ci_changes?
         changed_files(%r{\A(\.gitlab-ci\.yml|\.gitlab/ci/)}).any?
       end
+
+      def group_label(labels)
+        labels.find { |label| label.start_with?('group::') }
+      end
     end
   end
 end

tooling/gitlab_danger.rb

@@ -4,28 +4,29 @@
 class GitlabDanger
   LOCAL_RULES ||= %w[
     changes_size
+    commit_messages
+    database
     documentation
     duplicate_yarn_dependencies
-    prettier
     eslint
     karma
-    database
-    commit_messages
-    product_intelligence
-    utility_css
     pajamas
     pipeline
+    prettier
+    product_intelligence
+    utility_css
   ].freeze
 
   CI_ONLY_RULES ||= %w[
-    metadata
+    ce_ee_vue_templates
     changelog
-    specs
+    ci_templates
+    metadata
+    feature_flag
     roulette
-    ce_ee_vue_templates
     sidekiq_queues
     specialization_labels
-    ci_templates
+    specs
   ].freeze
 
   MESSAGE_PREFIX = '==>'.freeze