Add Danger checks for feature flag YAML definition files

Signed-off-by: Rémy Coutable <remy@rymai.me>

danger/feature_flag/Dangerfile

 # frozen_string_literal: true
 
-FF_SEE_DOC = "See the [feature flag documentation](https://docs.gitlab.com/ee/development/feature_flags#feature-flag-definition-and-validation)."
-FEATURE_FLAG_LABEL = "feature flag"
-FEATURE_FLAG_EXISTS_LABEL = "#{FEATURE_FLAG_LABEL}::exists"
-FEATURE_FLAG_SKIPPED_LABEL = "#{FEATURE_FLAG_LABEL}::skipped"
-DEVOPS_LABELS_REQUIRING_FEATURE_FLAG_REVIEW = ["devops::verify"]
-
-FF_SUGGEST_MR_COMMENT = <<~SUGGEST_COMMENT
-```suggestion
-group: "%<group>s"
-```
-
-#{FF_SEE_DOC}
-SUGGEST_COMMENT
-
-FEATURE_FLAG_ENFORCEMENT_WARNING = <<~WARNING_MESSAGE
-There were no new or modified feature flag YAML files detected in this MR.
-
-If the changes here are already controlled under an existing feature flag, please add
-the ~"#{FEATURE_FLAG_EXISTS_LABEL}". Otherwise, if you think the changes here don't need
-to be under a feature flag, please add the label ~"#{FEATURE_FLAG_SKIPPED_LABEL}", and
-add a short comment about why we skipped the feature flag.
-
-For guidance on when to use a feature flag, please see the [documentation](https://about.gitlab.com/handbook/product-development-flow/feature-flag-lifecycle/#when-to-use-feature-flags).
-WARNING_MESSAGE
-
-def check_feature_flag_yaml(feature_flag)
-  mr_group_label = helper.group_label
-
-  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 "#{helper.html_link(feature_flag.path)} isn't valid YAML! #{FF_SEE_DOC}"
-rescue StandardError => e
-  warn "There was a problem trying to check the Feature Flag file. Exception: #{e.class.name} - #{e.message}"
-end
+module Tooling
+  class FeatureFlagDangerfile
+    SEE_DOC = "See the [feature flag documentation](https://docs.gitlab.com/ee/development/feature_flags#feature-flag-definition-and-validation)."
+    FEATURE_FLAG_LABEL = "feature flag"
+    FEATURE_FLAG_EXISTS_LABEL = "#{FEATURE_FLAG_LABEL}::exists".freeze
+    FEATURE_FLAG_SKIPPED_LABEL = "#{FEATURE_FLAG_LABEL}::skipped".freeze
+    DEVOPS_LABELS_REQUIRING_FEATURE_FLAG_REVIEW = ["devops::verify"].freeze
+
+    SUGGEST_MR_COMMENT = <<~SUGGEST_COMMENT.freeze
+    ```suggestion
+    group: "%<group>s"
+    ```
+
+    #{SEE_DOC}
+    SUGGEST_COMMENT
+
+    FEATURE_FLAG_ENFORCEMENT_WARNING = <<~WARNING_MESSAGE.freeze
+    There were no new or modified feature flag YAML files detected in this MR.
+
+    If the changes here are already controlled under an existing feature flag, please add
+    the ~"#{FEATURE_FLAG_EXISTS_LABEL}". Otherwise, if you think the changes here don't need
+    to be under a feature flag, please add the label ~"#{FEATURE_FLAG_SKIPPED_LABEL}", and
+    add a short comment about why we skipped the feature flag.
+
+    For guidance on when to use a feature flag, please see the [documentation](https://about.gitlab.com/handbook/product-development-flow/feature-flag-lifecycle/#when-to-use-feature-flags).
+    WARNING_MESSAGE
+
+    def initialize(context:, feature_flag_helper:, helper:)
+      @context = context
+      @feature_flag_helper = feature_flag_helper
+      @helper = helper
+    end
 
-def message_for_feature_flag_missing_group!(feature_flag:, mr_group_label:)
-  if mr_group_label.nil?
-    warn "Consider setting `group` in #{helper.html_link(feature_flag.path)}. #{FF_SEE_DOC}"
-  else
-    mr_line = feature_flag.raw.lines.find_index("group:\n")
+    def check_feature_flag_yaml(feature_flag)
+      unless feature_flag.valid?
+        context.failure("#{helper.html_link(feature_flag.path)} isn't valid YAML! #{SEE_DOC}")
+        return
+      end
+
+      check_group(feature_flag)
+      check_feature_issue_url(feature_flag)
+      # Note: we don't check introduced_by_url as it's already done by danger/config_files/Dangerfile
+      check_rollout_issue_url(feature_flag)
+      check_milestone(feature_flag)
+      check_default_enabled(feature_flag)
+    end
 
-    if mr_line
-      markdown(format(FF_SUGGEST_MR_COMMENT, group: mr_group_label), file: feature_flag.path, line: mr_line.succ)
-    else
-      warn %(Consider setting `group: "#{mr_group_label}"` in #{helper.html_link(feature_flag.path)}. #{FF_SEE_DOC})
+    def check_touched_feature_flag_files
+      touched_feature_flag_files.each do |feature_flag|
+        check_feature_flag_yaml(feature_flag)
+      end
     end
-  end
-end
 
-def message_for_global_rollout(feature_flag)
-  return unless feature_flag.default_enabled == true
+    def mr_has_backend_or_frontend_changes?
+      changes = helper.changes_by_category
+      changes.has_key?(:backend) || changes.has_key?(:frontend)
+    end
 
-  message = <<~SUGGEST_COMMENT
-  You're about to [release the feature with the feature flag](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/issue_templates/Feature%20Flag%20Roll%20Out.md#optional-release-the-feature-with-the-feature-flag).
-  This process can only be done **after** the [global rollout on production](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/issue_templates/Feature%20Flag%20Roll%20Out.md#global-rollout-on-production).
-  Please make sure in [the rollout issue](#{feature_flag.rollout_issue_url}) that the preliminary steps have already been done. Otherwise, changing the YAML definition might not have the desired effect.
-  SUGGEST_COMMENT
+    def stage_requires_feature_flag_review?
+      DEVOPS_LABELS_REQUIRING_FEATURE_FLAG_REVIEW.include?(helper.stage_label)
+    end
 
-  mr_line = feature_flag.raw.lines.find_index { |l| l.include?('default_enabled:') }
-  markdown(message, file: feature_flag.path, line: mr_line.succ)
-end
+    def mr_missing_feature_flag_status_label?
+      ([FEATURE_FLAG_EXISTS_LABEL, FEATURE_FLAG_SKIPPED_LABEL] & helper.mr_labels).none?
+    end
 
-def message_for_feature_flag_with_group!(feature_flag:, mr_group_label:)
-  return if feature_flag.group_match_mr_label?(mr_group_label)
+    private
 
-  if mr_group_label.nil?
-    helper.labels_to_add << feature_flag.group
-  else
-    fail %(`group` is set to ~"#{feature_flag.group}" in #{helper.html_link(feature_flag.path)}, which does not match ~"#{mr_group_label}" set on the MR!)
-  end
-end
+    attr_reader :context, :feature_flag_helper, :helper
 
-def added_feature_flag_files
-  feature_flag.feature_flag_files(change_type: :added)
-end
+    def touched_feature_flag_files
+      added_feature_flag_files + modified_feature_flag_files
+    end
 
-def modified_feature_flag_files
-  feature_flag.feature_flag_files(change_type: :modified)
-end
+    def added_feature_flag_files
+      feature_flag_helper.feature_flag_files(change_type: :added)
+    end
 
-def feature_flag_file_added?
-  added_feature_flag_files.any?
-end
+    def modified_feature_flag_files
+      feature_flag_helper.feature_flag_files(change_type: :modified)
+    end
 
-def feature_flag_file_modified?
-  modified_feature_flag_files.any?
-end
+    def feature_flag_file_touched?
+      added_feature_flag_files.any? || modified_feature_flag_files.any?
+    end
 
-def feature_flag_file_added_or_modified?
-  feature_flag_file_added? || feature_flag_file_modified?
-end
+    def check_group(feature_flag)
+      mr_group_label = helper.group_label
 
-def mr_has_backend_or_frontend_changes?
-  changes = helper.changes_by_category
-  changes.has_key?(:backend) || changes.has_key?(:frontend)
-end
+      if feature_flag.missing_group?
+        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
+    end
 
-def mr_missing_feature_flag_status_label?
-  ([FEATURE_FLAG_EXISTS_LABEL, FEATURE_FLAG_SKIPPED_LABEL] & helper.mr_labels).none?
-end
+    def message_for_feature_flag_missing_group!(feature_flag:, mr_group_label:)
+      if mr_group_label.nil?
+        context.failure("Please specify a valid `group` label in #{helper.html_link(feature_flag.path)}. #{SEE_DOC}")
+        return
+      end
 
-def stage_requires_feature_flag_review?
-  DEVOPS_LABELS_REQUIRING_FEATURE_FLAG_REVIEW.include?(helper.stage_label)
-end
+      mr_line = feature_flag.find_line_index("group:")
 
-added_feature_flag_files.each do |feature_flag|
-  check_feature_flag_yaml(feature_flag)
-end
+      if mr_line
+        context.failure(format(SUGGEST_MR_COMMENT, group: mr_group_label), file: feature_flag.path, line: mr_line.succ)
+      else
+        context.failure(%(Please add `group: "#{mr_group_label}"` in #{helper.html_link(feature_flag.path)}. #{SEE_DOC}))
+      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?
+        helper.labels_to_add << feature_flag.group
+      else
+        note = <<~FAILURE_MESSAGE
+        `group` is set to ~"#{feature_flag.group}" in #{helper.html_link(feature_flag.path)},
+        which does not match ~"#{mr_group_label}" set on the MR!
+        FAILURE_MESSAGE
+        mr_line = feature_flag.find_line_index("group:")
+
+        if mr_line
+          context.failure(note, file: feature_flag.path, line: mr_line.succ)
+        else
+          context.failure(note)
+        end
+      end
+    end
 
-modified_feature_flag_files.each do |feature_flag|
-  message_for_global_rollout(feature_flag)
+    def check_feature_issue_url(feature_flag)
+      return unless feature_flag.missing_feature_issue_url?
+
+      note = "Consider filling `feature_issue_url:`"
+      mr_line = feature_flag.find_line_index("feature_issue_url:")
+
+      if mr_line
+        context.message(note, file: feature_flag.path, line: mr_line.succ)
+      else
+        context.message(note)
+      end
+    end
+
+    def check_rollout_issue_url(feature_flag)
+      return unless ::Feature::Shared::TYPES.dig(feature_flag.name.to_sym, :rollout_issue)
+      return unless feature_flag.missing_rollout_issue_url?
+
+      missing_field_error(feature_flag: feature_flag, field: :rollout_issue_url)
+    end
+
+    def check_milestone(feature_flag)
+      return unless feature_flag.missing_milestone?
+
+      missing_field_error(feature_flag: feature_flag, field: :milestone)
+    end
+
+    def check_default_enabled(feature_flag)
+      return unless feature_flag.default_enabled?
+
+      if ::Feature::Shared::TYPES.dig(feature_flag.type.to_sym, :can_be_default_enabled)
+        note = <<~SUGGEST_COMMENT
+          You're about to [release the feature with the feature flag](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/issue_templates/Feature%20Flag%20Roll%20Out.md#optional-release-the-feature-with-the-feature-flag).
+          This process can only be done **after** the [global rollout on production](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/issue_templates/Feature%20Flag%20Roll%20Out.md#global-rollout-on-production).
+          Please make sure in [the rollout issue](#{feature_flag.rollout_issue_url}) that the preliminary steps have already been done. Otherwise, changing the YAML definition might not have the desired effect.
+        SUGGEST_COMMENT
+
+        mr_line = feature_flag.find_line_index("default_enabled: true")
+        context.markdown(note, file: feature_flag.path, line: mr_line.succ) if mr_line
+      else
+        context.failure(
+          "[Feature flag with the `#{feature_flag.type}` type must not be enabled by default](https://docs.gitlab.com/ee/development/feature_flags/##{feature_flag.type}-type). " \
+          "Consider changing the feature flag type if it's ready to be enabled by default."
+        )
+      end
+    end
+
+    def missing_field_error(feature_flag:, field:)
+      note = <<~MISSING_FIELD_ERROR
+        [Feature flag with the `#{feature_flag.type}` type must have `:#{field}` set](https://docs.gitlab.com/ee/development/feature_flags/##{feature_flag.type}-type).
+      MISSING_FIELD_ERROR
+      mr_line = feature_flag.find_line_index("#{field}:")
+
+      if mr_line
+        context.message(note, file: feature_flag.path, line: mr_line.succ)
+      else
+        context.message(note)
+      end
+    end
+  end
 end
 
-if helper.security_mr? && feature_flag_file_added?
-  fail "Feature flags are discouraged from security merge requests. Read the [security documentation](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/security/utilities/feature_flags.md) for details."
+feature_flag_dangerfile = Tooling::FeatureFlagDangerfile.new(context: self, feature_flag_helper: feature_flag, helper: helper)
+
+feature_flag_dangerfile.check_touched_feature_flag_files
+
+if helper.security_mr? && feature_flag_dangerfile.feature_flag_file_touched?
+  failure("Feature flags are discouraged from security merge requests. Read the [security documentation](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/security/utilities/feature_flags.md) for details.")
 end
 
-if !helper.security_mr? && mr_has_backend_or_frontend_changes? && stage_requires_feature_flag_review?
-  if feature_flag_file_added_or_modified? && !helper.mr_has_labels?(FEATURE_FLAG_EXISTS_LABEL)
+if !helper.security_mr? && feature_flag_dangerfile.mr_has_backend_or_frontend_changes? && feature_flag_dangerfile.stage_requires_feature_flag_review?
+  if feature_flag_dangerfile.feature_flag_file_touched? && !helper.mr_has_labels?(Tooling::FeatureFlagDangerfile::FEATURE_FLAG_EXISTS_LABEL)
     # Feature flag config file touched in this MR, so let's add the label to avoid the warning.
-    helper.labels_to_add << FEATURE_FLAG_EXISTS_LABEL
+    helper.labels_to_add << Tooling::FeatureFlagDangerfile::FEATURE_FLAG_EXISTS_LABEL
   end
 
-  warn FEATURE_FLAG_ENFORCEMENT_WARNING if mr_missing_feature_flag_status_label?
+  if feature_flag_dangerfile.mr_missing_feature_flag_status_label?
+    warn(Tooling::FeatureFlagDangerfile::FEATURE_FLAG_ENFORCEMENT_WARNING)
+  end
 end

spec/tooling/danger/feature_flag_spec.rb

 # frozen_string_literal: true
 
-require 'fast_spec_helper'
+require 'spec_helper'
 require 'gitlab/dangerfiles/spec_helper'
 
 require_relative '../../../tooling/danger/feature_flag'
 
-RSpec.describe Tooling::Danger::FeatureFlag do
+RSpec.describe Tooling::Danger::FeatureFlag, feature_category: :tooling do
   include_context "with dangerfile"
 
   let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
 
+  before do
+    allow(File).to receive(:read).and_return(YAML.dump({}))
+  end
+
   subject(:feature_flag) { fake_danger.new(helper: fake_helper) }
 
   describe '#feature_flag_files' do
@@ -85,39 +89,141 @@
 
   describe described_class::Found do
     let(:feature_flag_path) { 'config/feature_flags/development/entry.yml' }
+    let(:name) { 'entry' }
     let(:group) { 'group::source code' }
+    let(:default_enabled) { false }
+    let(:feature_issue_url) { 'https://gitlab.com/gitlab-org/gitlab/-/issues/1' }
+    let(:introduced_by_url) { 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/3' }
+    let(:rollout_issue_url) { 'https://gitlab.com/gitlab-org/gitlab/-/issues/2' }
+    let(:milestone) { '15.9' }
     let(:yaml) do
       {
+        'name' => name,
+        'default_enabled' => default_enabled,
+        'feature_issue_url' => feature_issue_url,
+        'rollout_issue_url' => rollout_issue_url,
+        'introduced_by_url' => introduced_by_url,
+        'milestone' => milestone,
         'group' => group,
-        'default_enabled' => true,
-        'rollout_issue_url' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/1',
-        'introduced_by_url' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/2',
-        'milestone' => '15.9',
-        'type' => 'development',
-        'name' => 'entry'
+        'type' => 'beta'
       }
     end
 
     let(:raw_yaml) { YAML.dump(yaml) }
 
-    subject(:found) { described_class.new(feature_flag_path) }
+    subject(:found) { described_class.build(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)
+      allow(File).to receive(:read).with(feature_flag_path).and_return(raw_yaml)
     end
 
-    described_class::ATTRIBUTES.each do |attribute|
-      describe "##{attribute}" do
-        it 'returns value from the YAML' do
-          expect(found.public_send(attribute)).to eq(yaml[attribute])
+    describe '.build' do
+      it { expect(found).to be_a(described_class) }
+
+      context 'when given path does not exist' do
+        before do
+          allow(File).to receive(:read).with(feature_flag_path).and_raise("File not found")
         end
+
+        it { expect(found.lines).to be_nil }
+      end
+
+      context 'when YAML is invalid' do
+        let(:raw_yaml) { 'foo bar' }
+
+        it { expect(found.lines).to be_nil }
+      end
+    end
+
+    describe '#valid?' do
+      context 'when name is nil' do
+        let(:name) { nil }
+
+        it { expect(found.valid?).to eq(false) }
+      end
+
+      context 'when name is not nil' do
+        it { expect(found.valid?).to eq(true) }
+      end
+    end
+
+    describe '#missing_group?' do
+      context 'when group is nil' do
+        let(:group) { nil }
+
+        it { expect(found.missing_group?).to eq(true) }
+      end
+
+      context 'when group is not nil' do
+        it { expect(found.missing_group?).to eq(false) }
+      end
+    end
+
+    describe '#missing_feature_issue_url?' do
+      context 'when feature_issue_url is nil' do
+        let(:feature_issue_url) { nil }
+
+        it { expect(found.missing_feature_issue_url?).to eq(true) }
+      end
+
+      context 'when feature_issue_url is not nil' do
+        it { expect(found.missing_feature_issue_url?).to eq(false) }
+      end
+    end
+
+    describe '#missing_introduced_by_url?' do
+      context 'when introduced_by_url is nil' do
+        let(:introduced_by_url) { nil }
+
+        it { expect(found.missing_introduced_by_url?).to eq(true) }
+      end
+
+      context 'when introduced_by_url is not nil' do
+        it { expect(found.missing_introduced_by_url?).to eq(false) }
+      end
+    end
+
+    describe '#missing_rollout_issue_url?' do
+      context 'when rollout_issue_url is nil' do
+        let(:rollout_issue_url) { nil }
+
+        it { expect(found.missing_rollout_issue_url?).to eq(true) }
+      end
+
+      context 'when rollout_issue_url is not nil' do
+        it { expect(found.missing_rollout_issue_url?).to eq(false) }
+      end
+    end
+
+    describe '#missing_milestone?' do
+      context 'when milestone is nil' do
+        let(:milestone) { nil }
+
+        it { expect(found.missing_milestone?).to eq(true) }
+      end
+
+      context 'when milestone is not nil' do
+        it { expect(found.missing_milestone?).to eq(false) }
       end
     end
 
-    describe '#raw' do
-      it 'returns the raw YAML' do
-        expect(found.raw).to eq(raw_yaml)
+    describe '#default_enabled?' do
+      context 'when default_enabled is nil' do
+        let(:default_enabled) { nil }
+
+        it { expect(found.default_enabled?).to eq(false) }
+      end
+
+      context 'when default_enabled is false' do
+        let(:default_enabled) { false }
+
+        it { expect(found.default_enabled?).to eq(false) }
+      end
+
+      context 'when default_enabled is true' do
+        let(:default_enabled) { true }
+
+        it { expect(found.default_enabled?).to eq(true) }
       end
     end
 
@@ -138,5 +244,17 @@
         end
       end
     end
+
+    describe '#find_line_index' do
+      context 'when line is found' do
+        let(:group) { nil }
+
+        it { expect(found.find_line_index("name: #{name}")).to eq(1) }
+      end
+
+      context 'when line is not found' do
+        it { expect(found.find_line_index("foo")).to be_nil }
+      end
+    end
   end
 end

tooling/danger/feature_flag.rb

 # frozen_string_literal: true
 
 require 'yaml'
+require_relative '../../lib/feature/shared'
 
 module Tooling
   module Danger
@@ -12,36 +13,71 @@ module FeatureFlag
       def feature_flag_files(change_type:)
         files = helper.public_send("#{change_type}_files") # rubocop:disable GitlabSecurity/PublicSend
 
-        files.select { |path| path =~ %r{\A(ee/)?config/feature_flags/} }.map { |path| Found.new(path) }
+        files.select { |path| path =~ %r{\A(ee/)?config/feature_flags/} }.map { |path| Found.build(path) }
       end
 
-      class Found
-        ATTRIBUTES = %w[name introduced_by_url rollout_issue_url milestone type group default_enabled].freeze
+      Found = Struct.new(
+        :path,
+        :lines,
+        :name,
+        :feature_issue_url,
+        :introduced_by_url,
+        :rollout_issue_url,
+        :milestone,
+        :group,
+        :type,
+        :default_enabled,
+        keyword_init: true
+      ) do
+        def self.build(path)
+          raw = File.read(path)
+          yaml = YAML.safe_load(raw)
 
-        attr_reader :path
+          new(path: path, lines: raw.lines, **yaml)
+        rescue Psych::Exception, StandardError
+          new(path: path)
+        end
+
+        def valid?
+          !missing_field?(:name)
+        end
 
-        def initialize(path)
-          @path = path
+        def missing_group?
+          missing_field?(:group)
         end
 
-        ATTRIBUTES.each do |attribute|
-          define_method(attribute) do
-            yaml[attribute]
-          end
+        def missing_feature_issue_url?
+          missing_field?(:feature_issue_url)
         end
 
-        def raw
-          @raw ||= File.read(path)
+        def missing_introduced_by_url?
+          missing_field?(:introduced_by_url)
+        end
+
+        def missing_rollout_issue_url?
+          missing_field?(:rollout_issue_url)
+        end
+
+        def missing_milestone?
+          missing_field?(:milestone)
+        end
+
+        def default_enabled?
+          !!default_enabled
         end
 
         def group_match_mr_label?(mr_group_label)
           mr_group_label == group
         end
 
+        def find_line_index(text)
+          lines.find_index("#{text}\n")
+        end
+
         private
 
-        def yaml
-          @yaml ||= YAML.safe_load(raw)
+        def missing_field?(field)
+          self[field].nil? || self[field].empty?
         end
       end
     end

tooling/danger/project_helper.rb

@@ -6,7 +6,6 @@ module ProjectHelper
       CI_ONLY_RULES = %w[
         ce_ee_vue_templates
         datateam
-        feature_flag
         master_pipeline_status
         roulette
         sidekiq_queues