Merge branch '50345-hashed-storage-feature-flag' into 'master'

Feature flag to disable Hashed Storage migration when renaming a repository

Closes #50345

See merge request gitlab-org/gitlab-ce!21291

app/models/project.rb

@@ -2072,13 +2072,19 @@ class Project < ActiveRecord::Base
   private
 
   def rename_or_migrate_repository!
-    if Gitlab::CurrentSettings.hashed_storage_enabled? && storage_version != LATEST_STORAGE_VERSION
+    if Gitlab::CurrentSettings.hashed_storage_enabled? &&
+        storage_upgradable? &&
+        Feature.disabled?(:skip_hashed_storage_upgrade) # kill switch in case we need to disable upgrade behavior
       ::Projects::HashedStorageMigrationService.new(self, full_path_was).execute
     else
       storage.rename_repo
     end
   end
 
+  def storage_upgradable?
+    storage_version != LATEST_STORAGE_VERSION
+  end
+
   def after_rename_repository(full_path_before, path_before)
     execute_rename_repository_hooks!(full_path_before)
 

changelogs/unreleased/50345-hashed-storage-feature-flag.yml

@@ -0,0 +1,5 @@
+---
+title: Feature flag to disable Hashed Storage migration when renaming a repository
+merge_request: 21291
+author:
+type: added

doc/development/feature_flags.md

@@ -57,3 +57,15 @@ end
 Features that are developed and are intended to be merged behind a feature flag
 should not include a changelog entry. The entry should be added in the merge
 request removing the feature flags.
+
+### Specs
+
+In the test environment `Feature.enabled?` is stubbed to always respond to `true`,
+so we make sure behavior under feature flag doesn't go untested in some non-specific
+contexts.
+ 
+If you need to test the feature flag in a different state, you need to stub it with: 
+
+```ruby
+stub_feature_flags(my_feature_flag: false)
+```

lib/feature.rb

@@ -47,7 +47,8 @@ class Feature
     end
 
     def disabled?(key, thing = nil)
-      !enabled?(key, thing)
+      # we need to make different method calls to make it easy to mock / define expectations in test mode
+      thing.nil? ? !enabled?(key) : !enabled?(key, thing)
     end
 
     def enable(key, thing = true)

spec/lib/feature_spec.rb

@@ -1,6 +1,13 @@
 require 'spec_helper'
 
 describe Feature do
+  before do
+    # We mock all calls to .enabled? to return true in order to force all
+    # specs to run the feature flag gated behavior, but here we need a clean
+    # behavior from the class
+    allow(described_class).to receive(:enabled?).and_call_original
+  end
+
   describe '.get' do
     let(:feature) { double(:feature) }
     let(:key) { 'my_feature' }
@@ -106,4 +113,63 @@ describe Feature do
       it_behaves_like 'a memoized Flipper instance'
     end
   end
+
+  describe '.enabled?' do
+    it 'returns false for undefined feature' do
+      expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey
+    end
+
+    it 'returns false for existing disabled feature in the database' do
+      described_class.disable(:disabled_feature_flag)
+
+      expect(described_class.enabled?(:disabled_feature_flag)).to be_falsey
+    end
+
+    it 'returns true for existing enabled feature in the database' do
+      described_class.enable(:enabled_feature_flag)
+
+      expect(described_class.enabled?(:enabled_feature_flag)).to be_truthy
+    end
+
+    context 'with an individual actor' do
+      CustomActor = Struct.new(:flipper_id)
+
+      let(:actor) { CustomActor.new(flipper_id: 'CustomActor:5') }
+      let(:another_actor) { CustomActor.new(flipper_id: 'CustomActor:10') }
+
+      before do
+        described_class.enable(:enabled_feature_flag, actor)
+      end
+
+      it 'returns true when same actor is informed' do
+        expect(described_class.enabled?(:enabled_feature_flag, actor)).to be_truthy
+      end
+
+      it 'returns false when different actor is informed' do
+        expect(described_class.enabled?(:enabled_feature_flag, another_actor)).to be_falsey
+      end
+
+      it 'returns false when no actor is informed' do
+        expect(described_class.enabled?(:enabled_feature_flag)).to be_falsey
+      end
+    end
+  end
+
+  describe '.disable?' do
+    it 'returns true for undefined feature' do
+      expect(described_class.disabled?(:some_random_feature_flag)).to be_truthy
+    end
+
+    it 'returns true for existing disabled feature in the database' do
+      described_class.disable(:disabled_feature_flag)
+
+      expect(described_class.disabled?(:disabled_feature_flag)).to be_truthy
+    end
+
+    it 'returns false for existing enabled feature in the database' do
+      described_class.enable(:enabled_feature_flag)
+
+      expect(described_class.disabled?(:enabled_feature_flag)).to be_falsey
+    end
+  end
 end

spec/models/project_spec.rb

@@ -2989,6 +2989,7 @@ describe Project do
         # call. This makes testing a bit easier.
         allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
         allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
+        stub_feature_flags(skip_hashed_storage_upgrade: false)
       end
 
       it 'renames a repository' do
@@ -3160,6 +3161,7 @@ describe Project do
         # call. This makes testing a bit easier.
         allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
         allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
+        stub_feature_flags(skip_hashed_storage_upgrade: false)
       end
 
       context 'migration to hashed storage' do

spec/services/projects/update_service_spec.rb

@@ -249,9 +249,20 @@ describe Projects::UpdateService do
           expect(project.errors.messages[:base]).to include('There is already a repository with that name on disk')
         end
 
-        context 'when hashed storage enabled' do
+        it 'renames the project without upgrading it' do
+          result = update_project(project, admin, path: 'new-path')
+
+          expect(result).not_to include(status: :error)
+          expect(project).to be_valid
+          expect(project.errors).to be_empty
+          expect(project.disk_path).to include('new-path')
+          expect(project.reload.hashed_storage?(:repository)).to be_falsey
+        end
+
+        context 'when hashed storage is enabled' do
           before do
             stub_application_setting(hashed_storage_enabled: true)
+            stub_feature_flags(skip_hashed_storage_upgrade: false)
           end
 
           it 'migrates project to a hashed storage instead of renaming the repo to another legacy name' do
@@ -262,6 +273,22 @@ describe Projects::UpdateService do
             expect(project.errors).to be_empty
             expect(project.reload.hashed_storage?(:repository)).to be_truthy
           end
+
+          context 'when skip_hashed_storage_upgrade feature flag is enabled' do
+            before do
+              stub_feature_flags(skip_hashed_storage_upgrade: true)
+            end
+
+            it 'renames the project without upgrading it' do
+              result = update_project(project, admin, path: 'new-path')
+
+              expect(result).not_to include(status: :error)
+              expect(project).to be_valid
+              expect(project.errors).to be_empty
+              expect(project.disk_path).to include('new-path')
+              expect(project.reload.hashed_storage?(:repository)).to be_falsey
+            end
+          end
         end
       end
 

spec/support/helpers/stub_feature_flags.rb

@@ -1,4 +1,7 @@
 module StubFeatureFlags
+  # Stub Feature flags with `flag_name: true/false`
+  #
+  # @param [Hash] features where key is feature name and value is boolean whether enabled or not
   def stub_feature_flags(features)
     features.each do |feature_name, enabled|
       allow(Feature).to receive(:enabled?).with(feature_name) { enabled }