Improved Milestones and add a test for Keeps::DeleteOldFeatureFlags

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

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]) }

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
 

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,

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

spec/keeps/delete_old_feature_flags_spec.rb

+# 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

spec/keeps/helpers/milestones_spec.rb

 # 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