From 1ecdf6df2b984618ae0078c73b152475172e75d8 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Tue, 5 Dec 2023 10:14:56 -0800 Subject: [PATCH] Create feature flag for Grouped security updates (#8529) --- ...eate_group_security_update_pull_request.rb | 1 + ...resh_group_security_update_pull_request.rb | 1 + .../dependabot/file_fetcher_command_spec.rb | 7 +++- updater/spec/dependabot/integration_spec.rb | 10 +++-- .../dependabot/update_files_command_spec.rb | 10 ++--- .../dependabot/updater/error_handler_spec.rb | 30 +++++++-------- .../dependabot/updater/operations_spec.rb | 38 +++++++++++++++++++ updater/spec/dependabot/updater_spec.rb | 12 +++++- 8 files changed, 81 insertions(+), 28 deletions(-) diff --git a/updater/lib/dependabot/updater/operations/create_group_security_update_pull_request.rb b/updater/lib/dependabot/updater/operations/create_group_security_update_pull_request.rb index 2ae5099e16..c8a0efb590 100644 --- a/updater/lib/dependabot/updater/operations/create_group_security_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/create_group_security_update_pull_request.rb @@ -15,6 +15,7 @@ class CreateGroupSecurityUpdatePullRequest include GroupUpdateCreation def self.applies_to?(job:) + return false if Dependabot::Experiments.enabled?(:grouped_security_updates_disabled) return false if job.updating_a_pull_request? # If we haven't been given data for the vulnerable dependency, # this strategy cannot act. diff --git a/updater/lib/dependabot/updater/operations/refresh_group_security_update_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_group_security_update_pull_request.rb index 5a0d3088aa..34c36d610c 100644 --- a/updater/lib/dependabot/updater/operations/refresh_group_security_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_group_security_update_pull_request.rb @@ -17,6 +17,7 @@ class RefreshGroupSecurityUpdatePullRequest include GroupUpdateRefreshing def self.applies_to?(job:) + return false if Dependabot::Experiments.enabled?(:grouped_security_updates_disabled) # If we haven't been given data for the vulnerable dependency, # this strategy cannot act. return false unless job.dependencies&.any? diff --git a/updater/spec/dependabot/file_fetcher_command_spec.rb b/updater/spec/dependabot/file_fetcher_command_spec.rb index f335dfba06..ef9de7d051 100644 --- a/updater/spec/dependabot/file_fetcher_command_spec.rb +++ b/updater/spec/dependabot/file_fetcher_command_spec.rb @@ -132,7 +132,11 @@ allow_any_instance_of(Dependabot::Bundler::FileFetcher) .to receive(:commit) .and_raise(StandardError, "my_branch") - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) + Dependabot::Experiments.register(:record_update_job_unknown_error, true) + end + + after do + Dependabot::Experiments.reset! end it "tells the backend about the error via update job error api (and doesn't re-raise it)" do @@ -176,7 +180,6 @@ allow_any_instance_of(Dependabot::Bundler::FileFetcher) .to receive(:commit) .and_raise(StandardError, "my_branch") - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false) end it "tells the backend about the error via update job error api (and doesn't re-raise it)" do diff --git a/updater/spec/dependabot/integration_spec.rb b/updater/spec/dependabot/integration_spec.rb index ed3c30c1c4..94b9d22134 100644 --- a/updater/spec/dependabot/integration_spec.rb +++ b/updater/spec/dependabot/integration_spec.rb @@ -209,8 +209,12 @@ context "when there is an exception that blocks PR creation (cloud)" do before do allow(api_client).to receive(:create_pull_request).and_raise(StandardError, "oh no!") - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_ecosystem_versions).and_return(true) - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) + Dependabot::Experiments.register(:record_ecosystem_versions, true) + Dependabot::Experiments.register(:record_update_job_unknown_error, true) + end + + after do + Dependabot::Experiments.reset! end it "notifies Dependabot API of the problem" do @@ -258,8 +262,6 @@ context "when there is an exception that blocks PR creation (ghes)" do before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_ecosystem_versions).and_return(false) - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false) allow(api_client).to receive(:create_pull_request).and_raise(StandardError, "oh no!") end diff --git a/updater/spec/dependabot/update_files_command_spec.rb b/updater/spec/dependabot/update_files_command_spec.rb index 03992efe20..1cfaeab0a6 100644 --- a/updater/spec/dependabot/update_files_command_spec.rb +++ b/updater/spec/dependabot/update_files_command_spec.rb @@ -116,7 +116,11 @@ context "with an update files error (cloud)" do let(:error) { StandardError.new("hell") } before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) + Dependabot::Experiments.register(:record_update_job_unknown_error, true) + end + + after do + Dependabot::Experiments.reset! end it_behaves_like "a fast-failed job" @@ -160,10 +164,6 @@ context "with an update files error (ghes)" do let(:error) { StandardError.new("hell") } - before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false) - end - it_behaves_like "a fast-failed job" it "it captures the exception and records to a update job error api" do diff --git a/updater/spec/dependabot/updater/error_handler_spec.rb b/updater/spec/dependabot/updater/error_handler_spec.rb index 434d044939..1f4e2f6f0f 100644 --- a/updater/spec/dependabot/updater/error_handler_spec.rb +++ b/updater/spec/dependabot/updater/error_handler_spec.rb @@ -64,7 +64,11 @@ end before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) + Dependabot::Experiments.register(:record_update_job_unknown_error, true) + end + + after do + Dependabot::Experiments.reset! end it "records the error with both update job error api services, logs the backtrace and captures the exception" do @@ -115,10 +119,6 @@ end end - before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false) - end - it "records error with only update job error api service, logs the backtrace and captures the exception" do expect(mock_service).to_not receive(:record_update_job_unknown_error) @@ -162,7 +162,11 @@ end before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) + Dependabot::Experiments.register(:record_update_job_unknown_error, true) + end + + after do + Dependabot::Experiments.reset! end it "records the error with the service and logs the backtrace" do @@ -232,10 +236,6 @@ end end - before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false) - end - it "records the error with the service and logs the backtrace" do expect(mock_service).to receive(:record_update_job_error).with( error_type: "unknown_error", @@ -310,7 +310,11 @@ end before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) + Dependabot::Experiments.register(:record_update_job_unknown_error, true) + end + + after do + Dependabot::Experiments.reset! end it "records the error with the update job error services, logs the backtrace and captures the exception" do @@ -360,10 +364,6 @@ end end - before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false) - end - it "records the error with the update job error services, logs the backtrace and captures the exception" do expect(mock_service).to receive(:record_update_job_error).with( error_type: "unknown_error", diff --git a/updater/spec/dependabot/updater/operations_spec.rb b/updater/spec/dependabot/updater/operations_spec.rb index 34c5ae4323..bfb2266d7c 100644 --- a/updater/spec/dependabot/updater/operations_spec.rb +++ b/updater/spec/dependabot/updater/operations_spec.rb @@ -91,6 +91,44 @@ .to be(Dependabot::Updater::Operations::CreateSecurityUpdatePullRequest) end + it "returns the CreateGroupSecurityUpdatePullRequest class when Experiment flag is not provided" do + job = instance_double(Dependabot::Job, + security_updates_only?: true, + updating_a_pull_request?: false, + dependencies: [anything, anything], + dependency_groups: [anything], + is_a?: true) + + expect(described_class.class_for(job: job)) + .to be(Dependabot::Updater::Operations::CreateGroupSecurityUpdatePullRequest) + end + + it "returns the CreateGroupSecurityUpdatePullRequest class when Experiment flag is off" do + Dependabot::Experiments.register(:grouped_security_updates_disabled, false) + job = instance_double(Dependabot::Job, + security_updates_only?: true, + updating_a_pull_request?: false, + dependencies: [anything, anything], + dependency_groups: [anything], + is_a?: true) + + expect(described_class.class_for(job: job)) + .to be(Dependabot::Updater::Operations::CreateGroupSecurityUpdatePullRequest) + end + + it "returns the CreateSecurityUpdatePullRequest class when Experiment flag is true" do + Dependabot::Experiments.register(:grouped_security_updates_disabled, true) + job = instance_double(Dependabot::Job, + security_updates_only?: true, + updating_a_pull_request?: false, + dependencies: [anything, anything], + dependency_groups: [anything], + is_a?: true) + + expect(described_class.class_for(job: job)) + .to be(Dependabot::Updater::Operations::CreateSecurityUpdatePullRequest) + end + it "returns the RefreshGroupSecurityUpdatePullRequest class when the Job is for an existing security update for" \ " multiple dependencies" do job = instance_double(Dependabot::Job, diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index c98bdc9be7..89e3e51d1d 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/updater/spec/dependabot/updater_spec.rb @@ -1600,7 +1600,11 @@ def expect_update_checker_with_ignored_versions(versions, dependency_matcher: an context "when an unknown error is raised while updating dependencies (cloud) " do before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) + Dependabot::Experiments.register(:record_update_job_unknown_error, true) + end + + after do + Dependabot::Experiments.reset! end it "tells Sentry" do @@ -1983,7 +1987,11 @@ def expect_update_checker_with_ignored_versions(versions, dependency_matcher: an context "when an unknown error is raised while updating dependencies (ghes)" do before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false) + Dependabot::Experiments.register(:record_update_job_unknown_error, false) + end + + after do + Dependabot::Experiments.reset! end it "tells Sentry" do