Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create feature flag for Grouped security updates #8529

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Create feature flag for Grouped security updates #8529

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
449c197
Create feature flag for Grouped security updates
ryanbrandenburg Dec 4, 2023
e9ccc8c
Cleanup
ryanbrandenburg Dec 5, 2023
a5bc573
Merge branch 'main' into dev/rybrande/GroupUpdate
jakecoffman Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions updater/lib/dependabot/updater/operations/create_group_security_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class CreateGroupSecurityUpdatePullRequest
include GroupUpdateCreation

def self.applies_to?(job:)
return false if Dependabot::Experiments.enabled?(:grouped_security_updates_disabled)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recognize that it's a bit weird to phrase it like this, but I need the default case to be grouped_security_updates being on and the way Dependabot::Experiments works is it's falsy by default.

return false if job.updating_a_pull_request?
# If we haven't been given data for the vulnerable dependency,
# this strategy cannot act.
Expand Down
1 change: 1 addition & 0 deletions updater/lib/dependabot/updater/operations/refresh_group_security_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
7 changes: 5 additions & 2 deletions updater/spec/dependabot/file_fetcher_command_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions updater/spec/dependabot/integration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

@ryanbrandenburg ryanbrandenburg Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell doing it this way really only worked while we had a single call against enabled?. Since we already had the reset! method I figured we could make use of it instead of mocking things so that later levels of abstraction have an easier time handling things.

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
Expand Down Expand Up @@ -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

Expand Down
10 changes: 5 additions & 5 deletions updater/spec/dependabot/update_files_command_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
30 changes: 15 additions & 15 deletions updater/spec/dependabot/updater/error_handler_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -232,10 +236,6 @@
end
end

before do
allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These explicit mocks were not needed once we stop mocking (default to false), so I deleted them.

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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down
38 changes: 38 additions & 0 deletions updater/spec/dependabot/updater/operations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
12 changes: 10 additions & 2 deletions updater/spec/dependabot/updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down