From c14da78d3a4aff35a941dded9252280a2aec4732 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Thu, 13 Feb 2025 22:44:21 +0530 Subject: [PATCH 1/3] Ensure target_sha is always set, remove skip_target_sha Signed-off-by: Siddharth Asthana --- app/models/repository.rb | 5 +---- app/services/snippets/create_service.rb | 4 ---- spec/models/repository_spec.rb | 2 +- spec/services/snippets/create_service_spec.rb | 8 -------- 4 files changed, 2 insertions(+), 17 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index c6810bd89aeb59..fb028dda3d521b 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -901,10 +901,7 @@ 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 - end + options[:target_sha] = self.commit(options[:branch_name])&.sha || blank_ref with_cache_hooks { raw.commit_files(user, **options) } end diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 58da5195c44477..cd554f1055b070 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -124,9 +124,5 @@ def build_actions_from_params(_snippet) def restricted_files_actions :create end - - def commit_attrs(snippet, msg) - super.merge(skip_target_sha: true) - end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 24583107f1395a..dadfa6c4946482 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -4397,7 +4397,7 @@ def update_storages(storage_hash) context 'with an empty branch' do let_it_be(:project) { create(:project, :empty_repo) } - let(:target_sha) { nil } + let(:target_sha) { repository.blank_ref } it 'calls UserCommitFiles RPC' do expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |client| diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 501f847b953e29..2410bb2f5b690c 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -118,14 +118,6 @@ 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)) - end - - subject - end - context 'when repository creation action fails' do before do allow_next_instance_of(Snippet) do |instance| -- GitLab From c040f70c3adc9133ad506eb2d05cfe9013b62973 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Tue, 18 Feb 2025 12:17:48 +0530 Subject: [PATCH 2/3] Add feature flag commit_files_target_sha to control target_sha behavior --- app/models/repository.rb | 9 ++++- app/services/snippets/create_service.rb | 10 ++++++ .../commit_files_target_sha.yml | 9 +++++ spec/models/repository_spec.rb | 34 ++++++++++++++++--- spec/services/snippets/create_service_spec.rb | 28 +++++++++++++++ 5 files changed, 84 insertions(+), 6 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 fb028dda3d521b..3cf3879672c404 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -901,7 +901,14 @@ def commit_files(user, **options) options[:start_repository] = start_project.repository.raw_repository end - options[:target_sha] = self.commit(options[:branch_name])&.sha || blank_ref + 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) } end diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index cd554f1055b070..dcecbe0a870192 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -124,5 +124,15 @@ def build_actions_from_params(_snippet) def restricted_files_actions :create end + + def commit_attrs(snippet, msg) + attrs = super + + unless Feature.enabled?(:commit_files_target_sha, snippet.project) + attrs[:skip_target_sha] = true + end + + attrs + end 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 00000000000000..26c7901fcc59cf --- /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 dadfa6c4946482..2ed9137f5a3440 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) { 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) + 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/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 2410bb2f5b690c..d8fe2ac31c0a7f 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -118,6 +118,34 @@ expect(blob.data).to eq base_opts[:content] end + context 'when feature flag is enabled' do + before do + stub_feature_flags(commit_files_target_sha: true) + end + + 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 before do allow_next_instance_of(Snippet) do |instance| -- GitLab From d0d22d705276adb09edb23d2cf60a6536de0b04b Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Thu, 27 Feb 2025 16:28:57 +0530 Subject: [PATCH 3/3] Fix failing tests in DesignManagement & CI Configuration Signed-off-by: Siddharth Asthana --- app/services/snippets/create_service.rb | 8 ++------ .../design_management/save_designs_service_spec.rb | 2 +- .../ci_configuration/create_service_shared_examples.rb | 10 ---------- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index dcecbe0a870192..4a6d2ec3a2fb43 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -126,13 +126,9 @@ def restricted_files_actions end def commit_attrs(snippet, msg) - attrs = super + return super if Feature.enabled?(:commit_files_target_sha, snippet.project) - unless Feature.enabled?(:commit_files_target_sha, snippet.project) - attrs[:skip_target_sha] = true - end - - attrs + super.merge(skip_target_sha: true) 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 c8ea024282cdd2..006a029d192b69 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/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 16c828c72fe482..e5f87d924d7534 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