Implement apply .patch in Keeps::DeleteOldFeatureFlags

The `gitlab-housekeeper` currently has a very simple implementation of
automatically removing feature flags that are past their expected usage
date.

This MR introduces new functionality to `Keeps::DeleteOldFeatureFlags`
where it looks for a `.patch` file matching the same name as the `.yml`
file for the feature flag. If it exists it will apply the patch.

The tool will be used to fully automate the developer workflow for
removing feature flags. Instead of developers needing to remember to
remove their feature flag later they can just add a `.patch` file when
they first create the feature flag. Later on the `gitlab-housekeeper`
will come along and clean it up for them.

config/feature_flags/development/access_token_pagination.patch

+diff --git a/app/controllers/concerns/render_access_tokens.rb b/app/controllers/concerns/render_access_tokens.rb
+index 43e4686e66f9..80b4fc0a9673 100644
+--- a/app/controllers/concerns/render_access_tokens.rb
++++ b/app/controllers/concerns/render_access_tokens.rb
+@@ -6,10 +6,8 @@ module RenderAccessTokens
+   def active_access_tokens
+     tokens = finder(state: 'active', sort: 'expires_at_asc_id_desc').execute.preload_users
+ 
+-    if Feature.enabled?('access_token_pagination')
+-      tokens = tokens.page(page)
+-      add_pagination_headers(tokens)
+-    end
++    tokens = tokens.page(page)
++    add_pagination_headers(tokens)
+ 
+     represent(tokens)
+   end
+diff --git a/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb b/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb
+index ec46c4a9ed8b..41527b9824d0 100644
+--- a/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb
++++ b/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb
+@@ -46,18 +46,6 @@
+     end
+   end
+ 
+-  context "when access_token_pagination feature flag is disabled" do
+-    before do
+-      stub_feature_flags(access_token_pagination: false)
+-      create(:personal_access_token, user: access_token_user)
+-    end
+-
+-    it "returns all tokens in system" do
+-      get_access_tokens_with_page
+-      expect(assigns(:active_access_tokens).count).to eq(2)
+-    end
+-  end
+-
+   context "when tokens returned are ordered" do
+     let(:expires_1_day_from_now) { 1.day.from_now.to_date }
+     let(:expires_2_day_from_now) { 2.days.from_now.to_date }

doc/development/feature_flags/index.md

@@ -374,6 +374,22 @@ Feature flags **must** be used in the MR that introduces them. Not doing so caus
 [broken master](https://handbook.gitlab.com/handbook/engineering/workflow/#broken-master) scenario due
 to the `rspec:feature-flags` job that only runs on the `master` branch.
 
+### Optionally add a `.patch` file for automated removal of feature flags
+
+The [`gitlab-housekeeper`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-housekeeper) is able to automatically remove your feature flag code for you using the [`DeleteOldFeatureFlags` keep](https://gitlab.com/gitlab-org/gitlab/-/blob/master/keeps/delete_old_feature_flags.rb). The tool will run periodically and automatically clean up old feature flags from the code.
+
+For this tool to automatically remove the usages of the feature flag in your code you can add a `.patch` file alongside your feature flag YAML file. The file should be exactly the same name except using the `.patch` extension instead of the `.yml` extension.
+
+For example you can create a patch file for `config/feature_flags/beta/my_feature_flag.yml` using the following steps:
+
+1. Edit the code locally to remove the feature flag `my_feature_flag` usage assuming that the feature flag is already enabled and we are rolling forward
+1. Run `git diff > config/feature_flags/beta/my_feature_flag.patch`
+1. Undo the changes to the files where you removed the feature flag usage
+1. Commit this file `config/feature_flags/beta/my_feature_flag.patch` file to the branch where you are adding the feature flag
+
+Then in future the `gitlab-housekeeper` will automatically clean up your
+feature flag for you by applying this patch.
+
 ## List all the feature flags
 
 To [use ChatOps](../../ci/chatops/index.md) to output all the feature flags in an environment to Slack, you can use the `run feature list`

keeps/delete_old_feature_flags.rb

@@ -6,6 +6,7 @@
 require_relative '../config/environment'
 require_relative 'helpers/groups'
 require_relative 'helpers/milestones'
+require_relative 'helpers/git_diff_parser'
 
 module Keeps
   # This is an implementation of a ::Gitlab::Housekeeper::Keep. This keep will locate any featrure flag definition file
@@ -52,6 +53,12 @@ def prepare_change(feature_flag)
       change.title = "Delete the `#{feature_flag.name}` feature flag"
       change.identifiers = [self.class.name.demodulize, feature_flag.name]
 
+      FileUtils.rm(feature_flag.path)
+
+      change.changed_files = [feature_flag.path]
+
+      apply_patch_and_cleanup(feature_flag, change)
+
       # 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.
@@ -62,7 +69,7 @@ def prepare_change(feature_flag)
 
       #{feature_flag_default_enabled_note(feature_flag.default_enabled)}
 
-      <details><summary>Mentions of the feature flag (click to expand)</summary>
+      <details><summary>Remaining mentions of the feature flag (click to expand)</summary>
 
       ```
       #{feature_flag_grep(feature_flag.name)}
@@ -70,8 +77,8 @@ def prepare_change(feature_flag)
 
       </details>
 
-      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 possible 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 always capable of removing all references so you must check the diff and pipeline failures to confirm if there are any issues.
       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.
 
@@ -79,10 +86,6 @@ def prepare_change(feature_flag)
       MARKDOWN
       # rubocop:enable Gitlab/DocUrl
 
-      FileUtils.rm(feature_flag.path)
-
-      change.changed_files = [feature_flag.path]
-
       change.labels = [
         'maintenance::removal',
         'feature flag',
@@ -125,6 +128,34 @@ def feature_flag_grep(feature_flag_name)
         '--',
         *(GREP_IGNORE.map { |path| ":^#{path}" })
       )
+    rescue ::Gitlab::Housekeeper::Shell::Error
+      # git grep returns error status if nothing is found
+    end
+
+    def apply_patch_and_cleanup(feature_flag, change)
+      return unless patch_exists?(feature_flag)
+
+      change.changed_files << patch_path(feature_flag)
+      change.changed_files += extract_changed_files_from_patch(feature_flag)
+
+      apply_patch(feature_flag)
+      FileUtils.rm(patch_path(feature_flag))
+    end
+
+    def patch_exists?(feature_flag)
+      File.file?(patch_path(feature_flag))
+    end
+
+    def apply_patch(feature_flag)
+      Gitlab::Housekeeper::Shell.execute('git', 'apply', patch_path(feature_flag))
+    end
+
+    def patch_path(feature_flag)
+      feature_flag.path.sub(/.yml$/, '.patch')
+    end
+
+    def extract_changed_files_from_patch(feature_flag)
+      git_diff_parser_helper.all_changed_files(File.read(patch_path(feature_flag)))
     end
 
     def feature_flag_rollout_issue_url(feature_flag)
@@ -174,5 +205,9 @@ def groups_helper
     def milestones_helper
       @milestones_helper ||= ::Keeps::Helpers::Milestones.new
     end
+
+    def git_diff_parser_helper
+      @git_diff_parser_helper ||= ::Keeps::Helpers::GitDiffParser.new
+    end
   end
 end

keeps/helpers/git_diff_parser.rb

+# frozen_string_literal: true
+
+module Keeps
+  module Helpers
+    class GitDiffParser
+      def all_changed_files(diff)
+        result = Set.new
+        diff.each_line do |line|
+          match = line.match(%r{^diff --git a/(.*) b/(.*)$})
+          result.merge(match.captures) if match
+        end
+
+        result.to_a
+      end
+    end
+  end
+end

spec/keeps/delete_old_feature_flags_spec.rb

@@ -14,19 +14,22 @@
   end
 
   let(:feature_flag_name) { 'feature_flag_name' }
+  let(:tmp_dir) { Pathname(Dir.mktmpdir) }
   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
+    file_path = tmp_dir.join('feature_flag.yml')
+
+    File.write(file_path, {
+      name: feature_flag_name,
+      milestone: feature_flag_milestone,
+      group: groups.dig(:foo, :label)
+    }.to_yaml)
+
+    file_path.to_s
   end
 
+  let(:feature_flag_patch_path) { feature_flag_file.sub(/.yml$/, '.patch') }
+
   let(:milestones_helper) { instance_double(Keeps::Helpers::Milestones) }
 
   subject(:keep) { described_class.new }
@@ -34,16 +37,26 @@
   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(:all_feature_flag_files).and_return([feature_flag_file])
     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)
+
+    File.write(feature_flag_patch_path, <<~DIFF)
+    diff --git a/foobar.txt b/foobar.txt
+    index 2ef267e25bd6..0fecdb8e98f3 100644
+    --- a/foobar.txt
+    +++ b/foobar.txt
+    @@ -1 +1 @@
+    -some content
+    +some content updated
+    DIFF
   end
 
   after do
-    feature_flag_file.close
+    FileUtils.rm_rf(tmp_dir)
   end
 
   describe '#each_change' do
@@ -55,7 +68,11 @@
         feature_flag_name, '--', ':^locale/', ':^db/structure.sql'
       )
 
-      expect(FileUtils).to receive(:rm).with(feature_flag_file.path)
+      expect(FileUtils).to receive(:rm).with(feature_flag_file)
+      expect(Gitlab::Housekeeper::Shell).to receive(:execute).with(
+        'git', 'apply', feature_flag_patch_path
+      )
+      expect(FileUtils).to receive(:rm).with(feature_flag_patch_path)
 
       actual_changes = keep.each_change(&:itself)
 
@@ -66,7 +83,11 @@
       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.changed_files).to contain_exactly(
+        feature_flag_file,
+        feature_flag_patch_path,
+        'foobar.txt'
+      )
       expect(actual_change.reviewers).to eq(['@john_doe'])
       expect(actual_change.labels).to eq(['maintenance::removal', 'feature flag', groups.dig(:foo, :label)])
     end

spec/keeps/helpers/git_diff_parser_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require './keeps/helpers/git_diff_parser'
+
+RSpec.describe Keeps::Helpers::GitDiffParser, feature_category: :tooling do
+  let(:parser) { described_class.new }
+
+  describe '#all_changed_files' do
+    let(:diff) do
+      <<~DIFF
+      diff --git a/foo.txt b/bar.txt
+      similarity index 100%
+      rename from foo.txt
+      rename to bar.txt
+      diff --git a/boo.txt b/boo.txt
+      new file mode 100644
+      index 000000000000..e69de29bb2d1
+      diff --git a/boobar.txt b/boobar.txt
+      deleted file mode 100644
+      index 60b20b312e05..000000000000
+      --- a/boobar.txt
+      +++ /dev/null
+      @@ -1 +0,0 @@
+      -something blah blah
+      diff --git a/foobar.txt b/foobar.txt
+      index 2ef267e25bd6..0fecdb8e98f3 100644
+      --- a/foobar.txt
+      +++ b/foobar.txt
+      @@ -1 +1 @@
+      -some content
+      +some content updated
+      DIFF
+    end
+
+    it 'returns all the files mentioned in the diff' do
+      expect(parser.all_changed_files(diff)).to contain_exactly(
+        'foo.txt',
+        'bar.txt',
+        'foobar.txt',
+        'boo.txt',
+        'boobar.txt'
+      )
+    end
+  end
+end