diff --git a/updater/lib/dependabot/environment.rb b/updater/lib/dependabot/environment.rb index 135a4ae979c..e5eebc7f8ad 100644 --- a/updater/lib/dependabot/environment.rb +++ b/updater/lib/dependabot/environment.rb @@ -46,6 +46,14 @@ def self.job_definition @job_definition ||= JSON.parse(File.read(job_path)) end + # TODO: Remove + # + # This is purely to provide a stubbable method for tests to start disabling + # this codeline and it can be removed before merging this branch + def self.legacy_run_enabled? + true + end + private_class_method def self.environment_variable(variable_name, default = :_undefined) return ENV.fetch(variable_name, default) unless default == :_undefined diff --git a/updater/lib/dependabot/updater.rb b/updater/lib/dependabot/updater.rb index 6b7d8195239..1ab8caeac63 100644 --- a/updater/lib/dependabot/updater.rb +++ b/updater/lib/dependabot/updater.rb @@ -95,6 +95,8 @@ def run # This is the original logic within run, we currently fail over to this if # no Operation class exists for the given job. def legacy_run + raise Dependabot::NotImplemented unless Environment.legacy_run_enabled? + service.increment_metric("updater.started", tags: { operation: "Legacy" }) if job.updating_a_pull_request? Dependabot.logger.info("Starting PR update job for #{job.source.repo}") diff --git a/updater/lib/dependabot/updater/operations.rb b/updater/lib/dependabot/updater/operations.rb index c880531399a..9dfa7898aec 100644 --- a/updater/lib/dependabot/updater/operations.rb +++ b/updater/lib/dependabot/updater/operations.rb @@ -36,6 +36,17 @@ module Operations ] def self.class_for(job:) + # Let's not bother generating the string if debug is disabled + if Dependabot.logger.debug? + update_type = job.security_updates_only? ? "security" : "version" + update_verb = job.updating_a_pull_request? ? "refresh" : "create" + update_deps = job.dependencies&.any? ? job.dependencies.count : "all" + + Dependabot.logger.debug( + "Finding operation for a #{update_type} to #{update_verb} a Pull Request for #{update_deps} dependencies" + ) + end + raise ArgumentError, "Expected Dependabot::Job, got #{job.class}" unless job.is_a?(Dependabot::Job) OPERATIONS.find { |op| op.applies_to?(job: job) } diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index 0f641524c39..bbc246a4c79 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/updater/spec/dependabot/updater_spec.rb @@ -12,6 +12,8 @@ RSpec.describe Dependabot::Updater do before do + # TODO: Remove + allow(Dependabot::Environment).to receive(:legacy_run_enabled?) { false } allow(Dependabot.logger).to receive(:info) stub_request(:get, "https://index.rubygems.org/versions"). @@ -447,9 +449,9 @@ end context "when ignore conditions are set" do - def expect_update_checker_with_ignored_versions(versions) + def expect_update_checker_with_ignored_versions(versions, dependency_matcher: anything) expect(Dependabot::Bundler::UpdateChecker).to have_received(:new).with( - dependency: anything, + dependency: dependency_matcher, dependency_files: anything, repo_contents_path: anything, credentials: anything, @@ -461,12 +463,11 @@ def expect_update_checker_with_ignored_versions(versions) ).once end - describe "when ignores match the dependency name" do + describe "when ignores match the a dependency being checked" do it "passes ignored_versions to the update checker" do stub_update_checker job = build_job( - requested_dependencies: ["dummy-pkg-b"], ignore_conditions: [ { "dependency-name" => "dummy-pkg-b", @@ -529,14 +530,14 @@ def expect_update_checker_with_ignored_versions(versions) it "doesn't enable raised_on_ignore for ignore logging" do stub_update_checker - job = build_job(requested_dependencies: ["dummy-pkg-b"]) + job = build_job service = build_service updater = build_updater(service: service, job: job) updater.run expect(Dependabot::Bundler::UpdateChecker).to have_received(:new).with( - dependency: anything, + dependency: having_attributes(name: "dummy-pkg-b"), dependency_files: anything, repo_contents_path: anything, credentials: anything, @@ -554,7 +555,6 @@ def expect_update_checker_with_ignored_versions(versions) stub_update_checker job = build_job( - requested_dependencies: ["dummy-pkg-b"], ignore_conditions: [ { "dependency-name" => "dummy-pkg-b", @@ -568,7 +568,7 @@ def expect_update_checker_with_ignored_versions(versions) updater.run expect(Dependabot::Bundler::UpdateChecker).to have_received(:new).with( - dependency: anything, + dependency: having_attributes(name: "dummy-pkg-b"), dependency_files: anything, repo_contents_path: anything, credentials: anything, @@ -586,7 +586,6 @@ def expect_update_checker_with_ignored_versions(versions) stub_update_checker job = build_job( - requested_dependencies: ["dummy-pkg-b"], ignore_conditions: [ { "dependency-name" => "dummy-pkg-b", @@ -600,7 +599,7 @@ def expect_update_checker_with_ignored_versions(versions) updater.run expect(Dependabot::Bundler::UpdateChecker).to have_received(:new).with( - dependency: anything, + dependency: having_attributes(name: "dummy-pkg-b"), dependency_files: anything, repo_contents_path: anything, credentials: anything, @@ -618,7 +617,6 @@ def expect_update_checker_with_ignored_versions(versions) stub_update_checker job = build_job( - requested_dependencies: ["dummy-pkg-a"], ignore_conditions: [ { "dependency-name" => "dummy-pkg-b", @@ -631,7 +629,7 @@ def expect_update_checker_with_ignored_versions(versions) updater.run - expect_update_checker_with_ignored_versions([]) + expect_update_checker_with_ignored_versions([], dependency_matcher: having_attributes(name: "dummy-pkg-a")) end end @@ -640,7 +638,6 @@ def expect_update_checker_with_ignored_versions(versions) stub_update_checker job = build_job( - requested_dependencies: ["dummy-pkg-a"], ignore_conditions: [ { "dependency-name" => "dummy-pkg-*", @@ -653,7 +650,10 @@ def expect_update_checker_with_ignored_versions(versions) updater.run - expect_update_checker_with_ignored_versions([">= 0"]) + expect_update_checker_with_ignored_versions( + [">= 0"], + dependency_matcher: having_attributes(name: "dummy-pkg-a") + ) end end @@ -662,7 +662,6 @@ def expect_update_checker_with_ignored_versions(versions) stub_update_checker job = build_job( - requested_dependencies: ["dummy-pkg-b"], ignore_conditions: [ { "dependency-name" => "dummy-pkg-a", @@ -683,7 +682,10 @@ def expect_update_checker_with_ignored_versions(versions) updater.run - expect_update_checker_with_ignored_versions([">= 2.0.0, < 3", "> 1.1.0, < 1.2", ">= 1.2.a, < 2"]) + expect_update_checker_with_ignored_versions( + [">= 2.0.0, < 3", "> 1.1.0, < 1.2", ">= 1.2.a, < 2"], + dependency_matcher: having_attributes(name: "dummy-pkg-b") + ) end end end @@ -1163,6 +1165,39 @@ def expect_update_checker_with_ignored_versions(versions) updater.run end + context "when the dependency is a sub-dependency" do + it "still attempts to update the dependency" do + stub_update_checker(vulnerable?: true) + + job = build_job( + requested_dependencies: ["dummy-pkg-a"], + updating_a_pull_request: true + ) + service = build_service + updater = build_updater( + service: service, + job: job, + dependency_files: [ + Dependabot::DependencyFile.new( + name: "Gemfile", + content: fixture("bundler/original/sub_dep"), + directory: "/" + ), + Dependabot::DependencyFile.new( + name: "Gemfile.lock", + content: fixture("bundler/original/sub_dep.lock"), + directory: "/" + ) + ] + ) + + expect(Dependabot::Updater::Operations::RefreshVersionUpdatePullRequest).to receive(:new).and_call_original + expect(service).to receive(:create_pull_request).once + + updater.run + end + end + context "when the dependency isn't vulnerable in a security update" do it "closes the pull request" do stub_update_checker(vulnerable?: true) @@ -1341,7 +1376,17 @@ def expect_update_checker_with_ignored_versions(versions) end end - context "and the job is not to update a PR" do + # This scenario is not currently used in production, it only exists + # implicity due to the legacy code mode-switching within methods. + # + # This will be deprecated when `legacy_run` is removed but should be + # fairly trivial to bring back if required. + context "and the job is create a version PR" do + before do + # Permit the legacy_run method to be used + allow(Dependabot::Environment).to receive(:legacy_run_enabled?) { true } + end + it "only attempts to update dependencies on the specified list" do stub_update_checker @@ -1361,184 +1406,157 @@ def expect_update_checker_with_ignored_versions(versions) updater.run end + end - context "when the dependency doesn't appear in the parsed file" do - it "does not try to close any pull request" do - stub_update_checker + context "and the job is create a security PR" do + context "when the dependency is vulnerable" do + it "creates the pull request" do + stub_update_checker(vulnerable?: true) job = build_job( - requested_dependencies: ["removed_dependency"], + requested_dependencies: ["dummy-pkg-b"], + security_advisories: [ + { + "dependency-name" => "dummy-pkg-b", + "affected-versions" => ["1.1.0"] + } + ], + security_updates_only: true, updating_a_pull_request: false ) service = build_service updater = build_updater(service: service, job: job) - expect(service).to_not receive(:close_pull_request) + expect(service).to receive(:create_pull_request) updater.run end end - context "when the dependency name case doesn't match what's parsed" do - it "only attempts to update dependencies on the specified list" do - stub_update_checker + context "when the dependency is not allowed to update" do + it "does not create the pull request" do + stub_update_checker(vulnerable?: true) job = build_job( - requested_dependencies: ["Dummy-pkg-b"], - updating_a_pull_request: false + requested_dependencies: ["dummy-pkg-b"], + security_advisories: [ + { + "dependency-name" => "dummy-pkg-b", + "affected-versions" => ["1.1.0"] + } + ], + allowed_updates: [ + { + "dependency-type" => "development" + } + ], + security_updates_only: true ) service = build_service updater = build_updater(service: service, job: job) - expect(updater). - to receive(:check_and_create_pr_with_error_handling). - and_call_original - expect(updater). - to_not receive(:check_and_update_existing_pr_with_error_handling) - expect(service).to receive(:create_pull_request).once + expect(service).not_to receive(:create_pull_request) + expect(service).to receive(:record_update_job_error).with( + { + error_type: "all_versions_ignored", + error_details: { + "dependency-name": "dummy-pkg-b" + } + } + ) + expect(Dependabot.logger). + to receive(:info).with( + "Dependabot cannot update to the required version as all " \ + "versions were ignored for dummy-pkg-b" + ) updater.run end end - context "when the dependency is a sub-dependency" do - it "still attempts to update the dependency" do - stub_update_checker + context "when the dependency is no longer vulnerable" do + it "does not create pull request" do + stub_update_checker(vulnerable?: false) job = build_job( - requested_dependencies: ["dummy-pkg-a"], - updating_a_pull_request: false + requested_dependencies: ["dummy-pkg-b"], + security_advisories: [ + { + "dependency-name" => "dummy-pkg-b", + "affected-versions" => ["1.0.0"], + "patched-versions" => ["1.1.0"] + } + ], + security_updates_only: true ) service = build_service - updater = build_updater( - service: service, - job: job, - dependency_files: [ - Dependabot::DependencyFile.new( - name: "Gemfile", - content: fixture("bundler/original/sub_dep"), - directory: "/" - ), - Dependabot::DependencyFile.new( - name: "Gemfile.lock", - content: fixture("bundler/original/sub_dep.lock"), - directory: "/" - ) - ] - ) + updater = build_updater(service: service, job: job) - expect(updater). - to receive(:check_and_create_pr_with_error_handling). - and_call_original - expect(updater). - to_not receive(:check_and_update_existing_pr_with_error_handling) - expect(service).to receive(:create_pull_request).once + expect(service).to_not receive(:create_pull_request) + expect(service).to receive(:record_update_job_error).with( + { + error_type: "security_update_not_needed", + error_details: { + "dependency-name": "dummy-pkg-b" + } + } + ) + expect(Dependabot.logger). + to receive(:info).with( + "no security update needed as dummy-pkg-b " \ + "is no longer vulnerable" + ) updater.run end end - context "for security only updates" do - context "when the dependency is vulnerable" do - it "creates the pull request" do - stub_update_checker(vulnerable?: true) - - job = build_job( - requested_dependencies: ["dummy-pkg-b"], - security_advisories: [ - { - "dependency-name" => "dummy-pkg-b", - "affected-versions" => ["1.1.0"] - } - ], - security_updates_only: true, - updating_a_pull_request: false - ) - service = build_service - updater = build_updater(service: service, job: job) - - expect(service).to receive(:create_pull_request) - - updater.run - end - end - - context "when the dependency is not allowed to update" do - it "does not create the pull request" do - stub_update_checker(vulnerable?: true) - - job = build_job( - requested_dependencies: ["dummy-pkg-b"], - security_advisories: [ - { - "dependency-name" => "dummy-pkg-b", - "affected-versions" => ["1.1.0"] - } - ], - allowed_updates: [ - { - "dependency-type" => "development" - } - ], - security_updates_only: true - ) - service = build_service - updater = build_updater(service: service, job: job) + context "when the dependency doesn't appear in the parsed file" do + it "does not try to close any pull request" do + stub_update_checker(vulnerable?: true) - expect(service).not_to receive(:create_pull_request) - expect(service).to receive(:record_update_job_error).with( + job = build_job( + requested_dependencies: ["removed_dependency"], + security_advisories: [ { - error_type: "all_versions_ignored", - error_details: { - "dependency-name": "dummy-pkg-b" - } + "dependency-name" => "removed_dependency", + "affected-versions" => ["1.1.0"] } - ) - expect(Dependabot.logger). - to receive(:info).with( - "Dependabot cannot update to the required version as all " \ - "versions were ignored for dummy-pkg-b" - ) + ], + security_updates_only: true, + updating_a_pull_request: false + ) + service = build_service + updater = build_updater(service: service, job: job) - updater.run - end - end + expect(service).to_not receive(:close_pull_request) - context "when the dependency is no longer vulnerable" do - it "does not create pull request" do - stub_update_checker(vulnerable?: false) + updater.run + end + end - job = build_job( - requested_dependencies: ["dummy-pkg-b"], - security_advisories: [ - { - "dependency-name" => "dummy-pkg-b", - "affected-versions" => ["1.0.0"], - "patched-versions" => ["1.1.0"] - } - ], - security_updates_only: true - ) - service = build_service - updater = build_updater(service: service, job: job) + context "when the dependency name case doesn't match what's parsed" do + it "still updates dependencies on the specified list" do + stub_update_checker(vulnerable?: true) - expect(service).to_not receive(:create_pull_request) - expect(service).to receive(:record_update_job_error).with( + job = build_job( + requested_dependencies: ["Dummy-pkg-b"], + security_advisories: [ { - error_type: "security_update_not_needed", - error_details: { - "dependency-name": "dummy-pkg-b" - } + # TODO: Should advisory name matching be case-insensitive too? + "dependency-name" => "Dummy-pkg-b", + "affected-versions" => ["1.1.0"] } - ) - expect(Dependabot.logger). - to receive(:info).with( - "no security update needed as dummy-pkg-b " \ - "is no longer vulnerable" - ) + ], + security_updates_only: true, + updating_a_pull_request: false + ) + service = build_service + updater = build_updater(service: service, job: job) - updater.run - end + expect(service).to receive(:create_pull_request).once + + updater.run end end end diff --git a/updater/spec/spec_helper.rb b/updater/spec/spec_helper.rb index 8e1734df4f3..37d6bb129e0 100644 --- a/updater/spec/spec_helper.rb +++ b/updater/spec/spec_helper.rb @@ -53,6 +53,13 @@ def fixture(path) File.read(File.join("spec", "fixtures", path)) end + + # TODO: Remove once Updater#legacy_run is gone + config.before do + require "dependabot/environment" + + allow(Dependabot::Environment).to receive(:legacy_run_enabled?) { false } + end end VCR.configure do |config|