Skip to content
Snippets Groups Projects
Commit 8cbbbe55 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin :two:
Browse files

Use cached values from `ProtectedBranch` cache

* Contributes to #366724
* Follow-up for: !92934

* Feature flag: #370608

**Problem**

After testing new Protected branch cache with `dry_run: true` option, we
can start relying on it.

**Solution**

* Add feature flag to control `dry_run` status
parent fa4a8184
No related branches found
No related tags found
2 merge requests!95848Draft: Add service to bulk process alerts,!92937Use cached values from `ProtectedBranch` cache
......@@ -25,10 +25,12 @@ def self.protected_ref_accessible_to?(ref, user, project:, action:, protected_re
end
# Check if branch name is marked as protected in the system
def self.protected?(project, ref_name, dry_run: true)
def self.protected?(project, ref_name)
return true if project.empty_repo? && project.default_branch_protected?
return false if ref_name.blank?
dry_run = Feature.disabled?(:rely_on_protected_branches_cache, project)
new_cache_result = new_cache(project, ref_name, dry_run: dry_run)
return new_cache_result unless new_cache_result.nil?
......
---
name: rely_on_protected_branches_cache
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92937
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/370608
milestone: '15.4'
type: development
group: group::source code
default_enabled: false
......@@ -7,7 +7,7 @@
# for example, adding quick actions when creating the issue and checking DateTime formats on UI.
# Because this kind of spec takes more time to run there is no need to add new ones
# for each existing quick action unless they test something not tested by existing tests.
RSpec.describe 'Merge request > User uses quick actions', :js do
RSpec.describe 'Merge request > User uses quick actions', :js, :use_clean_rails_redis_caching do
include Spec::Support::Helpers::Features::NotesHelpers
let(:project) { create(:project, :public, :repository) }
......
......@@ -171,8 +171,8 @@
let_it_be(:project) { create(:project, :repository) }
let_it_be(:protected_branch) { create(:protected_branch, project: project, name: "“jawn”") }
let(:feature_flag) { true }
let(:dry_run) { true }
let(:use_new_cache_implementation) { true }
let(:rely_on_new_cache) { true }
shared_examples_for 'hash based cache implementation' do
it 'calls only hash based cache implementation' do
......@@ -182,19 +182,22 @@
expect(Rails.cache).not_to receive(:fetch)
described_class.protected?(project, 'missing-branch', dry_run: dry_run)
described_class.protected?(project, 'missing-branch')
end
end
before do
stub_feature_flags(hash_based_cache_for_protected_branches: feature_flag)
stub_feature_flags(hash_based_cache_for_protected_branches: use_new_cache_implementation)
stub_feature_flags(rely_on_protected_branches_cache: rely_on_new_cache)
allow(described_class).to receive(:matching).and_call_original
# the original call works and warms the cache
described_class.protected?(project, protected_branch.name, dry_run: dry_run)
described_class.protected?(project, protected_branch.name)
end
context 'Dry-run: true' do
context 'Dry-run: true (rely_on_protected_branches_cache is off, new hash-based is used)' do
let(:rely_on_new_cache) { false }
it 'recalculates a fresh value every time in order to check the cache is not returning stale data' do
expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).twice
......@@ -204,21 +207,21 @@
it_behaves_like 'hash based cache implementation'
end
context 'Dry-run: false' do
let(:dry_run) { false }
context 'Dry-run: false (rely_on_protected_branches_cache is enabled, new hash-based cache is used)' do
let(:rely_on_new_cache) { true }
it 'correctly invalidates a cache' do
expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).exactly(3).times.and_call_original
create_params = { name: 'bar', merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }] }
branch = ProtectedBranches::CreateService.new(project, project.owner, create_params).execute
expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true)
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
ProtectedBranches::UpdateService.new(project, project.owner, name: 'ber').execute(branch)
expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true)
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
ProtectedBranches::DestroyService.new(project, project.owner).execute(branch)
expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true)
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end
it_behaves_like 'hash based cache implementation'
......@@ -229,7 +232,7 @@
project.touch
described_class.protected?(project, protected_branch.name, dry_run: dry_run)
described_class.protected?(project, protected_branch.name)
end
end
......@@ -240,19 +243,19 @@
another_project = create(:project)
ProtectedBranches::CreateService.new(another_project, another_project.owner, name: 'bar').execute
described_class.protected?(project, protected_branch.name, dry_run: dry_run)
described_class.protected?(project, protected_branch.name)
end
end
it 'correctly uses the cached version' do
expect(described_class).not_to receive(:matching)
expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true)
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end
end
context 'when feature flag hash_based_cache_for_protected_branches is off' do
let(:feature_flag) { false }
let(:use_new_cache_implementation) { false }
it 'does not call hash based cache implementation' do
expect(ProtectedBranches::CacheService).not_to receive(:new)
......
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe Git::BranchPushService, services: true do
RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: true do
include RepoHelpers
let_it_be(:user) { create(:user) }
......
......@@ -75,7 +75,16 @@
end
context 'when the merge request branch is protected from force push' do
let!(:protected_branch) { create(:protected_branch, project: project, name: merge_request.source_branch, allow_force_push: false) }
let!(:protected_branch) do
ProtectedBranches::CreateService.new(
project,
user,
name: merge_request.source_branch,
allow_force_push: false,
push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }],
merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }]
).execute
end
it 'does not rebase the MR' do
add_note("/rebase")
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment