From 68c372a900976e0c36b2f46b96235c1acbc0317c Mon Sep 17 00:00:00 2001
From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Mon, 3 Mar 2025 16:54:35 +0000
Subject: [PATCH] Ensure target_sha is always set, remove skip_target_sha
Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
---
app/models/repository.rb | 10 ++++--
app/services/snippets/create_service.rb | 2 ++
.../commit_files_target_sha.yml | 9 +++++
spec/models/repository_spec.rb | 34 ++++++++++++++++---
.../save_designs_service_spec.rb | 2 +-
spec/services/snippets/create_service_spec.rb | 28 ++++++++++++---
.../create_service_shared_examples.rb | 10 ------
7 files changed, 72 insertions(+), 23 deletions(-)
create mode 100644 config/feature_flags/gitlab_com_derisk/commit_files_target_sha.yml
diff --git a/app/models/repository.rb b/app/models/repository.rb
index c6810bd89aeb5..3cf3879672c40 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -901,9 +901,13 @@ def commit_files(user, **options)
options[:start_repository] = start_project.repository.raw_repository
end
- skip_target_sha = options.delete(:skip_target_sha)
- unless skip_target_sha
- options[:target_sha] = self.commit(options[:branch_name])&.sha
+ if Feature.enabled?(:commit_files_target_sha, project)
+ options[:target_sha] = self.commit(options[:branch_name])&.sha || blank_ref
+ else
+ skip_target_sha = options.delete(:skip_target_sha)
+ unless skip_target_sha
+ options[:target_sha] = self.commit(options[:branch_name])&.sha
+ end
end
with_cache_hooks { raw.commit_files(user, **options) }
diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb
index 58da5195c4447..4a6d2ec3a2fb4 100644
--- a/app/services/snippets/create_service.rb
+++ b/app/services/snippets/create_service.rb
@@ -126,6 +126,8 @@ def restricted_files_actions
end
def commit_attrs(snippet, msg)
+ return super if Feature.enabled?(:commit_files_target_sha, snippet.project)
+
super.merge(skip_target_sha: true)
end
end
diff --git a/config/feature_flags/gitlab_com_derisk/commit_files_target_sha.yml b/config/feature_flags/gitlab_com_derisk/commit_files_target_sha.yml
new file mode 100644
index 0000000000000..26c7901fcc59c
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/commit_files_target_sha.yml
@@ -0,0 +1,9 @@
+---
+name: commit_files_target_sha
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/517122
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/181459
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/520058
+milestone: '17.10'
+group: group::source code
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 24583107f1395..2ed9137f5a344 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -4397,14 +4397,38 @@ def update_storages(storage_hash)
context 'with an empty branch' do
let_it_be(:project) { create(:project, :empty_repo) }
- let(:target_sha) { nil }
- it 'calls UserCommitFiles RPC' do
- expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client|
- expect(client).to receive(:user_commit_files).with(*expected_params)
+ context 'when feature flag is enabled' do
+ before do
+ stub_feature_flags(commit_files_target_sha: true)
end
- subject
+ let(:target_sha) { repository.blank_ref }
+
+ it 'calls UserCommitFiles RPC' do
+ expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client|
+ expect(client).to receive(:user_commit_files).with(*expected_params)
+ end
+
+ subject
+ end
+ end
+
+ context 'when feature flag is disabled' do
+ let(:project) { create(:project, :empty_repo) }
+ let(:target_sha) { nil }
+
+ before do
+ stub_feature_flags(commit_files_target_sha: false)
+ end
+
+ it 'calls UserCommitFiles RPC with nil target_sha' do
+ expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client|
+ expect(client).to receive(:user_commit_files).with(*expected_params)
+ end
+
+ subject
+ end
end
end
end
diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb
index c8ea024282cdd..006a029d192b6 100644
--- a/spec/services/design_management/save_designs_service_spec.rb
+++ b/spec/services/design_management/save_designs_service_spec.rb
@@ -366,7 +366,7 @@ def repository_exists
expect { service.execute }
.to change { issue.designs.count }.from(0).to(2)
.and change { DesignManagement::Version.count }.by(1)
- .and change { Gitlab::GitalyClient.get_request_count }.by(4)
+ .and change { Gitlab::GitalyClient.get_request_count }.by(5)
.and change { commit_count }.by(1)
.and trigger_internal_events(Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_DESIGNS_ADDED)
.twice.with(user: user, project: project, category: 'InternalEventTracking')
diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb
index 501f847b953e2..d8fe2ac31c0a7 100644
--- a/spec/services/snippets/create_service_spec.rb
+++ b/spec/services/snippets/create_service_spec.rb
@@ -118,12 +118,32 @@
expect(blob.data).to eq base_opts[:content]
end
- it 'passes along correct commit attributes' do
- expect_next_instance_of(Repository) do |repository|
- expect(repository).to receive(:commit_files).with(anything, a_hash_including(skip_target_sha: true))
+ context 'when feature flag is enabled' do
+ before do
+ stub_feature_flags(commit_files_target_sha: true)
end
- subject
+ it 'does not pass skip_target_sha' do
+ expect_next_instance_of(Repository) do |repository|
+ expect(repository).to receive(:commit_files).with(anything, hash_excluding(:skip_target_sha))
+ end
+
+ subject
+ end
+ end
+
+ context 'when feature flag is disabled' do
+ before do
+ stub_feature_flags(commit_files_target_sha: false)
+ end
+
+ it 'passes skip_target_sha as true' do
+ expect_next_instance_of(Repository) do |repository|
+ expect(repository).to receive(:commit_files).with(anything, a_hash_including(skip_target_sha: true))
+ end
+
+ subject
+ end
end
context 'when repository creation action fails' do
diff --git a/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb b/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
index 16c828c72fe48..e5f87d924d753 100644
--- a/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
+++ b/spec/support/shared_examples/services/security/ci_configuration/create_service_shared_examples.rb
@@ -149,16 +149,6 @@
end
end
- context 'when parsing existing ci config gives any other error' do
- let(:params) { {} }
- let_it_be(:repository) { project.repository }
-
- it 'is successful' do
- expect(repository).to receive(:commit).twice.and_return(nil)
- expect(result.status).to eq(:success)
- end
- end
-
unless skip_w_params
context 'with parameters' do
let(:params) { non_empty_params }
--
GitLab