Skip to content

Implement apply .patch in Keeps::DeleteOldFeatureFlags

Dylan Griffith requested to merge housekeeper-feature-flag-apply-patch into master

What does this MR do and why?

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.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

  1. Commit the config/feature_flags/development/access_token_pagination.patch file from this branch into your master branch (housekeeper always runs against master locally so you just need it committed to your local master to test this)
  2. Run bundle exec gitlab-housekeeper -k '::Keeps::DeleteOldFeatureFlags' -d
  3. See the following output:
bundle exec gitlab-housekeeper -k '::Keeps::DeleteOldFeatureFlags' -d
$ bundle exec gitlab-housekeeper -k '::Keeps::DeleteOldFeatureFlags' -d
Merge request URL: (known after create)
=> DeleteOldFeatureFlags: access_token_pagination
=> Title:
Delete the `access_token_pagination` feature flag

=> Description:
This feature flag was introduced in 15.2, which is more than 12 milestones ago.

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).

Rollout issue: https://gitlab.com/gitlab-org/gitlab/-/issues/366534

The feature flag isn't enabled by default. If it's enabled on GitLab.com, you should keep the feature-flag
code branch, otherwise, keep the other branch.


Mentions of the feature flag (click to expand)

```
app/controllers/concerns/render_access_tokens.rb
9:    if Feature.enabled?('access_token_pagination')

config/feature_flags/development/access_token_pagination.patch
9:-    if Feature.enabled?('access_token_pagination')
26:-  context "when access_token_pagination feature flag is disabled" do
28:-      stub_feature_flags(access_token_pagination: false)

config/feature_flags/development/access_token_pagination.yml
2:name: access_token_pagination

spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb
49:  context "when access_token_pagination feature flag is disabled" do
51:      stub_feature_flags(access_token_pagination: false)

```



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 ~"group::authentication" 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.

You can also see the status of the rollout by checking https://gitlab.com/gitlab-org/gitlab/-/issues/366534 and https://gitlab.com/gitlab-com/gl-infra/feature-flag-log/-/issues/?search=access_token_pagination&sort=created_date&state=all&label_name%5B%5D=host%3A%3Agitlab.com.

=> Attributes:
Labels: maintenance::removal, feature flag, group::authentication, automation:gitlab-housekeeper-authored
Reviewers: sgarg_gitlab

=> Diff:
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/config/feature_flags/development/access_token_pagination.patch b/config/feature_flags/development/access_token_pagination.patch
deleted file mode 100644
index 7baf7f76a49c..000000000000
--- a/config/feature_flags/development/access_token_pagination.patch
+++ /dev/null
@@ -1,40 +0,0 @@
-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 }
diff --git a/config/feature_flags/development/access_token_pagination.yml b/config/feature_flags/development/access_token_pagination.yml
deleted file mode 100644
index 9cc8cf68e08f..000000000000
--- a/config/feature_flags/development/access_token_pagination.yml
+++ /dev/null
@@ -1,8 +0,0 @@
----
-name: access_token_pagination
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91372
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/366534
-milestone: '15.2'
-type: development
-group: group::authentication
-default_enabled: false
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 }

Housekeeper created 1 MRs
Edited by Dylan Griffith

Merge request reports