From 2cf11abe1e1f1c5d673478030645da33f60e0db0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Mon, 19 Feb 2024 13:33:47 +0100
Subject: [PATCH] Improved Milestones and add a test for
Keeps::DeleteOldFeatureFlags
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
Guardfile | 3 +-
keeps/delete_old_feature_flags.rb | 102 +++++++++++---------
keeps/helpers/groups.rb | 3 +-
keeps/helpers/milestones.rb | 34 +++++--
spec/keeps/delete_old_feature_flags_spec.rb | 74 ++++++++++++++
spec/keeps/helpers/milestones_spec.rb | 98 ++++++++++++++++++-
6 files changed, 252 insertions(+), 62 deletions(-)
create mode 100644 spec/keeps/delete_old_feature_flags_spec.rb
diff --git a/Guardfile b/Guardfile
index cd4486db404b9..33990778c1ba7 100644
--- a/Guardfile
+++ b/Guardfile
@@ -6,7 +6,7 @@ require "guard/rspec/dsl"
cmd = ENV['GUARD_CMD'] || (ENV['SPRING'] ? 'spring rspec' : 'bundle exec rspec')
-directories %w[app ee lib rubocop tooling spec]
+directories %w[app ee keeps lib rubocop tooling spec]
rspec_context_for = proc do |context_path|
OpenStruct.new(to_s: "spec").tap do |rspec| # rubocop:disable Style/OpenStructUse
@@ -41,6 +41,7 @@ guard_setup = proc do |context_path|
watch(rspec.spec_files)
# Ruby files
+ watch(%r{^#{context_path}(keeps/.+)\.rb$}) { |m| rspec.spec.call(m[1]) }
watch(%r{^#{context_path}(lib/.+)\.rb$}) { |m| rspec.spec.call(m[1]) }
watch(%r{^#{context_path}(rubocop/.+)\.rb$}) { |m| rspec.spec.call(m[1]) }
watch(%r{^#{context_path}(tooling/.+)\.rb$}) { |m| rspec.spec.call(m[1]) }
diff --git a/keeps/delete_old_feature_flags.rb b/keeps/delete_old_feature_flags.rb
index 53b06cd8f7944..952245dc48fdd 100644
--- a/keeps/delete_old_feature_flags.rb
+++ b/keeps/delete_old_feature_flags.rb
@@ -4,8 +4,8 @@
require 'cgi'
require_relative '../config/environment'
-require_relative './helpers/groups'
-require_relative './helpers/milestones'
+require_relative 'helpers/groups'
+require_relative 'helpers/milestones'
module Keeps
# This is an implementation of a ::Gitlab::Housekeeper::Keep. This keep will locate any featrure flag definition file
@@ -28,70 +28,76 @@ class DeleteOldFeatureFlags < ::Gitlab::Housekeeper::Keep
FEATURE_FLAG_LOG_ISSUES_URL = "https://gitlab.com/gitlab-com/gl-infra/feature-flag-log/-/issues/?search=%<feature_flag_name>s&sort=created_date&state=all&label_name%%5B%%5D=host%%3A%%3Agitlab.com"
def each_change
- each_feature_flag do |feature_flag_yaml_file, feature_flag_definition|
- feature_flag = Feature::Definition.new(feature_flag_yaml_file, feature_flag_definition)
+ each_feature_flag do |feature_flag|
+ change = prepare_change(feature_flag)
- if feature_flag.milestone.nil?
- puts "#{feature_flag.name} has no milestone set!"
- next
- end
+ yield(change) if change
+ end
+ end
- next unless milestones_helper.before_cuttoff?(
- milestone: feature_flag.milestone,
- milestones_ago: CUTOFF_MILESTONE_OLD)
+ private
- change = ::Gitlab::Housekeeper::Change.new
- change.changelog_type = 'removed'
- change.title = "Delete the `#{feature_flag.name}` feature flag"
- change.identifiers = [self.class.name.demodulize, feature_flag.name]
+ def prepare_change(feature_flag)
+ if feature_flag.milestone.nil?
+ puts "#{feature_flag.name} has no milestone set!"
+ return
+ end
- # rubocop:disable Gitlab/DocUrl -- Not running inside rails application
- change.description = <<~MARKDOWN
- This feature flag was introduced in #{feature_flag.milestone}, which is more than #{CUTOFF_MILESTONE_OLD} milestones ago.
+ return unless milestones_helper.before_cuttoff?(
+ milestone: feature_flag.milestone,
+ milestones_ago: CUTOFF_MILESTONE_OLD)
- As part of our process we want to ensure [feature flags don't stay too long in the codebase](https://docs.gitlab.com/ee/development/feature_flags/#types-of-feature-flags).
+ change = ::Gitlab::Housekeeper::Change.new
+ change.changelog_type = 'removed'
+ change.title = "Delete the `#{feature_flag.name}` feature flag"
+ change.identifiers = [self.class.name.demodulize, feature_flag.name]
- Rollout issue: #{feature_flag_rollout_issue_url(feature_flag)}
+ # rubocop:disable Gitlab/DocUrl -- Not running inside rails application
+ change.description = <<~MARKDOWN
+ This feature flag was introduced in #{feature_flag.milestone}, which is more than #{CUTOFF_MILESTONE_OLD} milestones ago.
- #{feature_flag_default_enabled_note(feature_flag_definition[:default_enabled])}
+ As part of our process we want to ensure [feature flags don't stay too long in the codebase](https://docs.gitlab.com/ee/development/feature_flags/#types-of-feature-flags).
- <details><summary>Mentions of the feature flag (click to expand)</summary>
+ Rollout issue: #{feature_flag_rollout_issue_url(feature_flag)}
- ```
- #{feature_flag_grep(feature_flag.name)}
- ```
+ #{feature_flag_default_enabled_note(feature_flag.default_enabled)}
- </details>
+ <details><summary>Mentions of the feature flag (click to expand)</summary>
- It is likely that this MR will still need some changes to remove references to the feature flag in the code.
- At the moment the `gitlab-housekeeper` is not capable of removing references but we'll be adding that functionality next.
- It is the responsibility of ~"#{feature_flag.group}" to push those changes to this branch.
- If they are already removing this feature flag in another merge request then they can just close this merge request.
+ ```
+ #{feature_flag_grep(feature_flag.name)}
+ ```
- You can also see the status of the rollout by checking #{feature_flag_rollout_issue_url(feature_flag)} and #{format(FEATURE_FLAG_LOG_ISSUES_URL, feature_flag_name: feature_flag.name)}.
- MARKDOWN
- # rubocop:enable Gitlab/DocUrl
+ </details>
- FileUtils.rm(feature_flag_yaml_file)
+ It is likely that this MR will still need some changes to remove references to the feature flag in the code.
+ At the moment the `gitlab-housekeeper` is not capable of removing references but we'll be adding that functionality next.
+ It is the responsibility of ~"#{feature_flag.group}" to push those changes to this branch.
+ If they are already removing this feature flag in another merge request then they can just close this merge request.
- change.changed_files = [feature_flag_yaml_file]
+ You can also see the status of the rollout by checking #{feature_flag_rollout_issue_url(feature_flag)} and #{format(FEATURE_FLAG_LOG_ISSUES_URL, feature_flag_name: feature_flag.name)}.
+ MARKDOWN
+ # rubocop:enable Gitlab/DocUrl
- change.labels = [
- 'maintenance::removal',
- 'feature flag',
- feature_flag.group
- ]
+ FileUtils.rm(feature_flag.path)
- change.reviewers = assignees(feature_flag.rollout_issue_url)
+ change.changed_files = [feature_flag.path]
- if change.reviewers.empty?
- group_data = groups_helper.group_for_group_label(feature_flag.group)
+ change.labels = [
+ 'maintenance::removal',
+ 'feature flag',
+ feature_flag.group
+ ]
- change.reviewers = groups_helper.pick_reviewer(group_data, change.identifiers) if group_data
- end
+ change.reviewers = assignees(feature_flag.rollout_issue_url)
- yield(change)
+ if change.reviewers.empty?
+ group_data = groups_helper.group_for_group_label(feature_flag.group)
+
+ change.reviewers = groups_helper.pick_reviewer(group_data, change.identifiers) if group_data
end
+
+ change
end
def feature_flag_default_enabled_note(feature_flag_default_enabled)
@@ -151,7 +157,9 @@ def get_rollout_issue(rollout_issue_url)
def each_feature_flag
all_feature_flag_files.map do |f|
- yield(f, YAML.load_file(f, permitted_classes: [Symbol], symbolize_names: true))
+ yield(
+ Feature::Definition.new(f, YAML.load_file(f, permitted_classes: [Symbol], symbolize_names: true))
+ )
end
end
diff --git a/keeps/helpers/groups.rb b/keeps/helpers/groups.rb
index 362dc9ddd064b..2cbf2bc295757 100644
--- a/keeps/helpers/groups.rb
+++ b/keeps/helpers/groups.rb
@@ -3,6 +3,7 @@
module Keeps
module Helpers
class Groups
+ GROUPS_JSON_URL = "https://about.gitlab.com/groups.json"
Error = Class.new(StandardError)
def group_for_feature_category(category)
@@ -31,7 +32,7 @@ def groups
def fetch_groups
@groups_json ||= begin
- response = Gitlab::HTTP.get('https://about.gitlab.com/groups.json')
+ response = Gitlab::HTTP.get(GROUPS_JSON_URL)
unless (200..299).cover?(response.code)
raise Error,
diff --git a/keeps/helpers/milestones.rb b/keeps/helpers/milestones.rb
index a6e4397b3c871..a747dde835da2 100644
--- a/keeps/helpers/milestones.rb
+++ b/keeps/helpers/milestones.rb
@@ -3,12 +3,18 @@
module Keeps
module Helpers
class Milestones
+ RELEASES_YML_URL = "https://gitlab.com/gitlab-com/www-gitlab-com/-/blob/master/data/releases.yml"
Error = Class.new(StandardError)
+ Milestone = Struct.new(:version, :date, keyword_init: true)
def before_cuttoff?(milestone:, milestones_ago:)
Gem::Version.new(milestone) < Gem::Version.new(past_milestone(milestones_ago: milestones_ago))
end
+ def past_milestone(milestones_ago:)
+ milestones[current_milestone_index + milestones_ago].version
+ end
+
private
def current_milestone
@@ -16,19 +22,27 @@ def current_milestone
milestone.gsub(/^(\d+\.\d+).*$/, '\1').chomp
end
- def past_milestone(milestones_ago:)
- major, minor = current_milestone.split(".").map(&:to_i)
+ def current_milestone_index
+ milestones.index { |milestone| milestone.version == current_milestone }
+ end
- older_major =
- if minor >= milestones_ago
- major
- else
- major - (((milestones_ago - minor - 1) / 13) + 1)
- end
+ def milestones
+ @milestones ||= fetch_milestones.map do |milestone|
+ Milestone.new(**milestone)
+ end
+ end
- older_minor = (0..12).to_a[(minor - milestones_ago) % 13]
+ def fetch_milestones
+ @milestones_yaml ||= begin
+ response = Gitlab::HTTP.get(RELEASES_YML_URL)
+
+ unless (200..299).cover?(response.code)
+ raise Error,
+ "Failed to get group information with response code: #{response.code} and body:\n#{response.body}"
+ end
- [older_major, older_minor].join(".")
+ YAML.safe_load(response.body)
+ end
end
end
end
diff --git a/spec/keeps/delete_old_feature_flags_spec.rb b/spec/keeps/delete_old_feature_flags_spec.rb
new file mode 100644
index 0000000000000..2063d52b8df26
--- /dev/null
+++ b/spec/keeps/delete_old_feature_flags_spec.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require './keeps/delete_old_feature_flags'
+
+RSpec.describe Keeps::DeleteOldFeatureFlags, feature_category: :tooling do
+ let(:groups) do
+ {
+ foo: {
+ label: 'group::foo',
+ backend_engineers: ['@john_doe']
+ }
+ }
+ end
+
+ let(:feature_flag_name) { 'feature_flag_name' }
+ let(:feature_flag_milestone) { '15.8' }
+ let(:feature_flag_file) do
+ Tempfile.new('feature_flag.yml').tap do |file|
+ file.open
+ file.write({
+ name: feature_flag_name,
+ milestone: feature_flag_milestone,
+ group: groups.dig(:foo, :label)
+ }.to_yaml)
+ file.rewind
+ end
+ end
+
+ let(:milestones_helper) { instance_double(Keeps::Helpers::Milestones) }
+
+ subject(:keep) { described_class.new }
+
+ before do
+ stub_request(:get, Keeps::Helpers::Groups::GROUPS_JSON_URL).to_return(status: 200, body: groups.to_json)
+
+ allow(keep).to receive(:all_feature_flag_files).and_return([feature_flag_file.path])
+ allow(keep).to receive(:milestones_helper).and_return(milestones_helper)
+
+ allow(milestones_helper)
+ .to receive(:before_cuttoff?).with(milestone: feature_flag_milestone, milestones_ago: 12)
+ .and_return(true)
+ end
+
+ after do
+ feature_flag_file.close
+ end
+
+ describe '#each_change' do
+ let(:expected_change) { instance_double(Gitlab::Housekeeper::Change) }
+
+ it 'returns a Gitlab::Housekeeper::Change', :aggregate_failures do
+ expect(Gitlab::Housekeeper::Shell).to receive(:execute).with(
+ 'git', 'grep', '--heading', '--line-number', '--break',
+ feature_flag_name, '--', ':^locale/', ':^db/structure.sql'
+ )
+
+ expect(FileUtils).to receive(:rm).with(feature_flag_file.path)
+
+ actual_changes = keep.each_change(&:itself)
+
+ expect(actual_changes.size).to eq(1)
+
+ actual_change = actual_changes.first
+ expect(actual_change).to be_a(Gitlab::Housekeeper::Change)
+ expect(actual_change.changelog_type).to eq('removed')
+ expect(actual_change.title).to eq("Delete the `#{feature_flag_name}` feature flag")
+ expect(actual_change.identifiers).to eq([described_class.name.demodulize, feature_flag_name])
+ expect(actual_change.changed_files).to eq([feature_flag_file.path])
+ expect(actual_change.reviewers).to eq(['@john_doe'])
+ expect(actual_change.labels).to eq(['maintenance::removal', 'feature flag', groups.dig(:foo, :label)])
+ end
+ end
+end
diff --git a/spec/keeps/helpers/milestones_spec.rb b/spec/keeps/helpers/milestones_spec.rb
index 8ee2e29769ae9..67c66b347db42 100644
--- a/spec/keeps/helpers/milestones_spec.rb
+++ b/spec/keeps/helpers/milestones_spec.rb
@@ -1,12 +1,102 @@
# frozen_string_literal: true
-require 'fast_spec_helper'
+require 'spec_helper'
require './keeps/helpers/milestones'
RSpec.describe Keeps::Helpers::Milestones, feature_category: :tooling do
+ let(:milestones_yaml) do
+ <<~YAML
+ - version: '17.0'
+ date: '2024-05-16'
+ - version: '16.11'
+ date: '2024-04-18'
+ - version: '16.10'
+ date: '2024-03-21'
+ - version: '16.9'
+ date: '2024-02-15'
+ - version: '16.8'
+ date: '2024-01-18'
+ - version: '16.7'
+ date: '2023-12-21'
+ - version: '16.6'
+ date: '2023-11-16'
+ - version: '16.5'
+ date: '2023-10-22'
+ - version: '16.4'
+ date: '2023-09-22'
+ - version: '16.3'
+ date: '2023-08-22'
+ - version: '16.2'
+ date: '2023-07-22'
+ - version: '16.1'
+ date: '2023-06-22'
+ - version: '16.0'
+ date: '2023-05-22'
+ - version: '15.11'
+ date: '2023-04-22'
+ - version: '15.10'
+ date: '2023-03-22'
+ - version: '15.9'
+ date: '2023-02-22'
+ - version: '15.8'
+ date: '2023-01-22'
+ - version: '15.7'
+ date: '2022-12-22'
+ - version: '15.6'
+ date: '2022-11-22'
+ - version: '15.5'
+ date: '2022-10-22'
+ - version: '15.4'
+ date: '2022-09-22'
+ - version: '15.3'
+ date: '2022-08-22'
+ - version: '15.2'
+ date: '2022-07-22'
+ - version: '15.1'
+ date: '2022-06-22'
+ - version: '15.0'
+ date: '2022-05-22'
+ - version: '14.10'
+ date: '2022-04-22'
+ - version: '14.9'
+ date: '2022-03-22'
+ YAML
+ end
+
before do
allow(File).to receive(:read).and_call_original
allow(File).to receive(:read).with(File.expand_path('../../../VERSION', __dir__)).and_return('16.9.0-pre')
+ stub_request(:get, described_class::RELEASES_YML_URL).to_return(status: 200, body: milestones_yaml)
+ end
+
+ describe '#past_milestone' do
+ let(:milestone) { '16.9' }
+
+ subject(:past_milestone) { described_class.new.past_milestone(milestones_ago: milestones_ago) }
+
+ context 'when asking 1 milestone ago' do
+ let(:milestones_ago) { 1 }
+
+ it { is_expected.to eq('16.8') }
+ end
+
+ context 'when asking 9 milestones ago' do
+ let(:milestones_ago) { 9 }
+
+ it { is_expected.to eq('16.0') }
+ end
+
+ context 'when asking 10 milestones ago' do
+ let(:milestones_ago) { 10 }
+
+ it { is_expected.to eq('15.11') }
+ end
+
+ context 'when asking 22 milestones ago' do
+ let(:milestones_ago) { 22 }
+
+ it { is_expected.to eq('14.10') }
+ end
end
describe '#before_cuttoff?' do
@@ -17,13 +107,15 @@
it { is_expected.to eq(false) }
context 'when milestone is before cuttoff' do
- let(:milestone) { '15.9' }
+ let(:milestone) { '15.8' }
it { is_expected.to eq(true) }
end
context 'when milestone is more than 2 major versions before cuttoff' do
- let(:milestone) { '14.12' }
+ let(:milestone) { '14.10' }
+
+ subject(:before_cuttoff) { described_class.new.before_cuttoff?(milestone: milestone, milestones_ago: 21) }
it { is_expected.to eq(true) }
end
--
GitLab