Merge branch 'sdungarwal-ff-keep-improvemnts' into 'master'

Add intended_to_rollout_by to prevent automatic removal

See merge request gitlab-org/gitlab!191923



Merged-by: Dylan Griffith <dyl.griffith@gmail.com>
Approved-by: Dylan Griffith <dyl.griffith@gmail.com>
Reviewed-by: GitLab Duo <gitlab-duo@gitlab.com>
Co-authored-by: Siddharth Dungarwal <sdungarwal@gitlab.com>

ee/spec/lib/ee/api/entities/experiment_spec.rb

@@ -16,6 +16,7 @@
         feature_issue_url: nil,
         introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45840',
         rollout_issue_url: nil,
+        intended_to_rollout_by: nil,
         milestone: '13.7',
         type: 'experiment',
         group: 'group::acquisition',

ee/spec/requests/api/experiments_spec.rb

@@ -62,6 +62,7 @@
               feature_issue_url: nil,
               introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45840',
               rollout_issue_url: nil,
+              intended_to_rollout_by: nil,
               milestone: '13.7',
               type: 'experiment',
               group: 'group::acquisition',

keeps/delete_old_feature_flags.rb

@@ -43,7 +43,25 @@ def each_change
 
     private
 
+    def parse_date(date_string)
+      Date.parse(date_string)
+    rescue Date::Error
+      nil
+    end
+
     def can_remove_ff?(feature_flag, identifiers, latest_feature_flag_status)
+      intended_to_rollout_by_date = feature_flag.intended_to_rollout_by
+      if intended_to_rollout_by_date.present?
+        rollout_date = parse_date(intended_to_rollout_by_date)
+        if !rollout_date.nil? && rollout_date.future?
+          @logger.puts "#{feature_flag.name} cannot be removed, intended rollout date is #{intended_to_rollout_by_date}"
+          return false
+        elsif rollout_date.nil?
+          message = "#{feature_flag.name} intended_to_rollout_by #{intended_to_rollout_by_date}"
+          @logger.puts "#{message}, is ignored as it cannot be parsed."
+        end
+      end
+
       if feature_flag.milestone.nil?
         @logger.puts "#{feature_flag.name} has no milestone set!"
         return false
@@ -100,7 +118,7 @@ def build_description(feature_flag, latest_feature_flag_status)
       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.
+      If they are already removing this feature flag in another merge request then they can just close this merge request and add `intended_to_rollout_by` date in the yml file.
 
       ## TODO for the reviewers before merging this MR
       - [ ] See the status of the rollout by checking #{feature_flag_rollout_issue_url(feature_flag)}, #{format(FEATURE_FLAG_LOG_ISSUES_URL, feature_flag_name: feature_flag.name)}
@@ -297,7 +315,7 @@ def global_search_flag?(feature_flag)
     end
 
     def all_feature_flag_files
-      Dir.glob("{,ee/}config/feature_flags/{development,gitlab_com_derisk,ops,beta}/*.yml")
+      Dir.glob("{,ee/}config/feature_flags/{development,gitlab_com_derisk,beta}/*.yml")
     end
 
     def remove_feature_flag_prompts

lib/feature/shared.rb

@@ -98,6 +98,7 @@ module Shared
       type
       group
       default_enabled
+      intended_to_rollout_by
     ].freeze
 
     def self.can_be_default_enabled?(feature_flag_type)

spec/keeps/delete_old_feature_flags_spec.rb

@@ -64,7 +64,8 @@
         rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/123',
         default_enabled: false,
         group: groups.dig(:foo, :label),
-        path: feature_flag_file
+        path: feature_flag_file,
+        intended_to_rollout_by: nil
       )
     end
 
@@ -88,7 +89,8 @@
           rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/123',
           default_enabled: false,
           group: groups.dig(:foo, :label),
-          path: feature_flag_file
+          path: feature_flag_file,
+          intended_to_rollout_by: nil
         )
       end
 
@@ -142,7 +144,8 @@
           rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/123',
           default_enabled: true,
           group: groups.dig(:foo, :label),
-          path: feature_flag_file
+          path: feature_flag_file,
+          intended_to_rollout_by: nil
         )
       end
 
@@ -184,6 +187,78 @@
         expect(keep.send(:can_remove_ff?, feature_flag, identifiers, :disabled)).to be true
       end
     end
+
+    describe '#parse_date' do
+      it 'returns a date object for valid date strings' do
+        expect(keep.send(:parse_date, '2023-01-01')).to eq(Date.new(2023, 1, 1))
+      end
+
+      it 'returns nil for invalid date strings' do
+        expect(keep.send(:parse_date, '2020')).to be_nil
+        expect(keep.send(:parse_date, 'invalid')).to be_nil
+        expect(keep.send(:parse_date, 'February 31, 2023')).to be_nil
+      end
+    end
+
+    context 'when feature flag has a future intended_to_rollout_by date' do
+      let(:feature_flag) do
+        instance_double(
+          Feature::Definition,
+          name: feature_flag_name,
+          milestone: feature_flag_milestone,
+          rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/123',
+          default_enabled: false,
+          group: groups.dig(:foo, :label),
+          path: feature_flag_file,
+          intended_to_rollout_by: (Time.zone.today + 30).to_s
+        )
+      end
+
+      it 'returns false' do
+        expect(keep.send(:can_remove_ff?, feature_flag, identifiers, :enabled)).to be false
+      end
+    end
+
+    context 'when feature flag has a past intended_to_rollout_by date' do
+      let(:feature_flag) do
+        instance_double(
+          Feature::Definition,
+          name: feature_flag_name,
+          milestone: feature_flag_milestone,
+          rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/123',
+          default_enabled: false,
+          group: groups.dig(:foo, :label),
+          path: feature_flag_file,
+          intended_to_rollout_by: (Time.zone.today - 30).to_s
+        )
+      end
+
+      it 'returns true when other conditions are met' do
+        expect(keep.send(:can_remove_ff?, feature_flag, identifiers, :enabled)).to be true
+      end
+    end
+
+    context 'when feature flag has an invalid intended_to_rollout_by date' do
+      let(:feature_flag) do
+        instance_double(
+          Feature::Definition,
+          name: feature_flag_name,
+          milestone: feature_flag_milestone,
+          rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/123',
+          default_enabled: false,
+          group: groups.dig(:foo, :label),
+          path: feature_flag_file,
+          intended_to_rollout_by: '2020'
+        )
+      end
+
+      # When parse_date returns nil for an invalid date, it passes through
+      # the condition and allows removal
+      it 'returns true when other conditions are met' do
+        expect(keep.send(:parse_date, '2020')).to be_nil
+        expect(keep.send(:can_remove_ff?, feature_flag, identifiers, :enabled)).to be true
+      end
+    end
   end
 
   describe '#each_change' do