From 17772f74558e14ea4ea439e2de73257c0057ce4e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 3 May 2024 11:53:34 +0200
Subject: [PATCH] Improve architecture of danger/feature_flag/
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
danger/feature_flag/Dangerfile | 101 ++++++++++++-----------
lib/feature/shared.rb | 4 +
spec/lib/feature/shared_spec.rb | 17 ++++
spec/tooling/danger/feature_flag_spec.rb | 6 +-
tooling/danger/feature_flag.rb | 4 +-
5 files changed, 80 insertions(+), 52 deletions(-)
create mode 100644 spec/lib/feature/shared_spec.rb
diff --git a/danger/feature_flag/Dangerfile b/danger/feature_flag/Dangerfile
index 816b0ad201fd6..307c7d23b68c1 100644
--- a/danger/feature_flag/Dangerfile
+++ b/danger/feature_flag/Dangerfile
@@ -27,32 +27,23 @@ module Tooling
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:)
+ def initialize(context:, added_files:, modified_files:, helper:)
@context = context
- @feature_flag_helper = feature_flag_helper
+ @added_files = added_files
+ @modified_files = modified_files
@helper = helper
end
- 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
-
def check_touched_feature_flag_files
touched_feature_flag_files.each do |feature_flag|
check_feature_flag_yaml(feature_flag)
end
end
+ def feature_flag_file_touched?
+ touched_feature_flag_files.any?
+ end
+
def mr_has_backend_or_frontend_changes?
changes = helper.changes_by_category
changes.has_key?(:backend) || changes.has_key?(:frontend)
@@ -68,22 +59,24 @@ module Tooling
private
- attr_reader :context, :feature_flag_helper, :helper
+ attr_reader :context, :added_files, :modified_files, :helper
- def touched_feature_flag_files
- added_feature_flag_files + modified_feature_flag_files
- end
-
- def added_feature_flag_files
- feature_flag_helper.feature_flag_files(change_type: :added)
- end
+ 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
- def modified_feature_flag_files
- feature_flag_helper.feature_flag_files(change_type: :modified)
+ 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
- def feature_flag_file_touched?
- added_feature_flag_files.any? || modified_feature_flag_files.any?
+ def touched_feature_flag_files
+ added_files + modified_files
end
def check_group(feature_flag)
@@ -102,13 +95,13 @@ module Tooling
return
end
- mr_line = feature_flag.find_line_index("group:")
-
- 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
+ add_message_on_line(
+ feature_flag: feature_flag,
+ needle: "group:",
+ note: format(SUGGEST_MR_COMMENT, group: mr_group_label),
+ fallback_note: %(Please add `group: "#{mr_group_label}"` in #{helper.html_link(feature_flag.path)}. #{SEE_DOC}),
+ message_method: :failure
+ )
end
def message_for_feature_flag_with_group!(feature_flag:, mr_group_label:)
@@ -121,27 +114,36 @@ module Tooling
`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
+ add_message_on_line(
+ feature_flag: feature_flag,
+ needle: "group:",
+ note: note,
+ message_method: :failure
+ )
end
end
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:")
+ add_message_on_line(
+ feature_flag: feature_flag,
+ needle: "feature_issue_url:",
+ note: "Consider filling `feature_issue_url:`"
+ )
+ end
+
+ def add_message_on_line(feature_flag:, needle:, note:, fallback_note: note, message_method: :message)
+ mr_line = feature_flag.find_line_index(needle)
+ # rubocop:disable GitlabSecurity/PublicSend -- we allow calling context.message, context.warning & context.failure
if mr_line
- context.message(note, file: feature_flag.path, line: mr_line.succ)
+ context.public_send(message_method, note, file: feature_flag.path, line: mr_line.succ)
else
- context.message(note)
+ context.public_send(message_method, fallback_note)
end
+ # rubocop:enable GitlabSecurity/PublicSend
end
def check_rollout_issue_url(feature_flag)
@@ -160,7 +162,7 @@ module Tooling
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)
+ if ::Feature::Shared.can_be_default_enabled?(feature_flag.type)
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).
@@ -192,7 +194,12 @@ module Tooling
end
end
-feature_flag_dangerfile = Tooling::FeatureFlagDangerfile.new(context: self, feature_flag_helper: feature_flag, helper: helper)
+feature_flag_dangerfile = Tooling::FeatureFlagDangerfile.new(
+ context: self,
+ added_files: feature_flag.feature_flag_files(danger_helper: helper, change_type: :added),
+ modified_files: feature_flag.feature_flag_files(danger_helper: helper, change_type: :modified),
+ helper: helper
+)
feature_flag_dangerfile.check_touched_feature_flag_files
diff --git a/lib/feature/shared.rb b/lib/feature/shared.rb
index 87cb4de9b0e60..cc711874b3f72 100644
--- a/lib/feature/shared.rb
+++ b/lib/feature/shared.rb
@@ -99,5 +99,9 @@ module Shared
group
default_enabled
].freeze
+
+ def self.can_be_default_enabled?(feature_flag_type)
+ TYPES.dig(feature_flag_type.to_sym, :can_be_default_enabled)
+ end
end
end
diff --git a/spec/lib/feature/shared_spec.rb b/spec/lib/feature/shared_spec.rb
new file mode 100644
index 0000000000000..cd9616c67c4ca
--- /dev/null
+++ b/spec/lib/feature/shared_spec.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Feature::Shared, feature_category: :tooling do
+ describe '.can_be_default_enabled?' do
+ subject { described_class.can_be_default_enabled?(type) }
+
+ described_class::TYPES.each do |type, data|
+ context "when type is #{type}" do
+ let(:type) { type }
+
+ it { is_expected.to eq(data[:can_be_default_enabled]) }
+ end
+ end
+ end
+end
diff --git a/spec/tooling/danger/feature_flag_spec.rb b/spec/tooling/danger/feature_flag_spec.rb
index 0124414cd9520..7f3466a750652 100644
--- a/spec/tooling/danger/feature_flag_spec.rb
+++ b/spec/tooling/danger/feature_flag_spec.rb
@@ -33,14 +33,14 @@
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)
+ expect(feature_flag.feature_flag_files(danger_helper: fake_helper, 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(danger_helper: fake_helper, 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(change_type: change_type)).to be_empty
+ expect(feature_flag.feature_flag_files(danger_helper: fake_helper, change_type: change_type)).to be_empty
end
end
diff --git a/tooling/danger/feature_flag.rb b/tooling/danger/feature_flag.rb
index af249ff237dc0..2540cfc154022 100644
--- a/tooling/danger/feature_flag.rb
+++ b/tooling/danger/feature_flag.rb
@@ -10,8 +10,8 @@ module FeatureFlag
# - :added
# - :modified
# - :deleted
- def feature_flag_files(change_type:)
- files = helper.public_send("#{change_type}_files") # rubocop:disable GitlabSecurity/PublicSend
+ def feature_flag_files(danger_helper:, change_type:)
+ files = danger_helper.public_send("#{change_type}_files") # rubocop:disable GitlabSecurity/PublicSend -- we allow calling danger_helper.added_files & danger_helper.modified_files
files.select { |path| path =~ %r{\A(ee/)?config/feature_flags/} }.map { |path| Found.build(path) }
end
--
GitLab