From 921363fbf00b7ea6d3777a02e89428e724305f1f Mon Sep 17 00:00:00 2001 From: kbukum1 Date: Wed, 4 Sep 2024 08:50:10 -0700 Subject: [PATCH] added sending bundler v1 deprecation warning alert (#10485) --- updater/lib/dependabot/api_client.rb | 42 ++- updater/lib/dependabot/opentelemetry.rb | 23 ++ updater/lib/dependabot/service.rb | 15 + .../updater/group_update_creation.rb | 5 + .../create_security_update_pull_request.rb | 6 + .../refresh_security_update_pull_request.rb | 6 + .../refresh_version_update_pull_request.rb | 6 + .../updater/operations/update_all_versions.rb | 6 + .../updater/security_update_helpers.rb | 47 +++ updater/spec/dependabot/api_client_spec.rb | 41 +++ updater/spec/dependabot/service_spec.rb | 25 +- .../create_group_update_pull_request_spec.rb | 41 +-- ...reate_security_update_pull_request_spec.rb | 325 ++++++++++-------- .../operations/update_all_versions_spec.rb | 26 +- .../updater/pull_request_helpers_spec.rb | 91 +++++ 15 files changed, 534 insertions(+), 171 deletions(-) create mode 100644 updater/spec/dependabot/updater/pull_request_helpers_spec.rb diff --git a/updater/lib/dependabot/api_client.rb b/updater/lib/dependabot/api_client.rb index 65be0c1d5e5..24b408228b1 100644 --- a/updater/lib/dependabot/api_client.rb +++ b/updater/lib/dependabot/api_client.rb @@ -102,8 +102,11 @@ def close_pull_request(dependency_names, reason) sig { params(error_type: T.any(String, Symbol), error_details: T.nilable(T::Hash[T.untyped, T.untyped])).void } def record_update_job_error(error_type:, error_details:) ::Dependabot::OpenTelemetry.tracer.in_span("record_update_job_error", kind: :internal) do |_span| - ::Dependabot::OpenTelemetry.record_update_job_error(job_id: job_id, error_type: error_type, - error_details: error_details) + ::Dependabot::OpenTelemetry.record_update_job_error( + job_id: job_id, + error_type: error_type, + error_details: error_details + ) api_url = "#{base_url}/update_jobs/#{job_id}/record_update_job_error" body = { data: { @@ -123,6 +126,41 @@ def record_update_job_error(error_type:, error_details:) end end + sig do + params( + warn_type: T.any(String, Symbol), + warn_title: String, + warn_description: String + ).void + end + def record_update_job_warning(warn_type:, warn_title:, warn_description:) + ::Dependabot::OpenTelemetry.tracer.in_span("record_update_job_message", kind: :internal) do |_span| + ::Dependabot::OpenTelemetry.record_update_job_warning( + job_id: job_id, + warn_type: warn_type, + warn_title: warn_title, + warn_description: warn_description + ) + api_url = "#{base_url}/update_jobs/#{job_id}/record_update_job_warning" + body = { + data: { + "warn-type": warn_type, + "warn-title": warn_title, + "warn-description": warn_description + } + } + response = http_client.post(api_url, json: body) + raise ApiError, response.body if response.code >= 400 + rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError + retry_count ||= 0 + retry_count += 1 + raise if retry_count > 3 + + sleep(rand(3.0..10.0)) + retry + end + end + sig { params(error_type: T.any(Symbol, String), error_details: T.nilable(T::Hash[T.untyped, T.untyped])).void } def record_update_job_unknown_error(error_type:, error_details:) error_type = "unknown_error" if error_type.nil? diff --git a/updater/lib/dependabot/opentelemetry.rb b/updater/lib/dependabot/opentelemetry.rb index 3d89b654628..9a933b9f61f 100644 --- a/updater/lib/dependabot/opentelemetry.rb +++ b/updater/lib/dependabot/opentelemetry.rb @@ -10,6 +10,9 @@ module OpenTelemetry module Attributes JOB_ID = "dependabot.job.id" + WARN_TYPE = "dependabot.job.warn_type" + WARN_TITLE = "dependabot.job.warn_title" + WARN_DESCRIPTION = "dependabot.job.warn_description" ERROR_TYPE = "dependabot.job.error_type" ERROR_DETAILS = "dependabot.job.error_details" METRIC = "dependabot.metric" @@ -89,6 +92,26 @@ def self.record_update_job_error(job_id:, error_type:, error_details:) current_span.add_event(error_type, attributes: attributes) end + sig do + params( + job_id: T.any(String, Integer), + warn_type: T.any(String, Symbol), + warn_title: String, + warn_description: String + ).void + end + def self.record_update_job_warning(job_id:, warn_type:, warn_title:, warn_description:) + current_span = ::OpenTelemetry::Trace.current_span + + attributes = { + Attributes::JOB_ID => job_id, + Attributes::WARN_TYPE => warn_type, + Attributes::WARN_TITLE => warn_title, + Attributes::WARN_DESCRIPTION => warn_description + } + current_span.add_event(warn_type, attributes: attributes) + end + sig do params( error: StandardError, diff --git a/updater/lib/dependabot/service.rb b/updater/lib/dependabot/service.rb index 5e69e3d27a8..c8cb0f2e5d3 100644 --- a/updater/lib/dependabot/service.rb +++ b/updater/lib/dependabot/service.rb @@ -81,6 +81,21 @@ def record_update_job_error(error_type:, error_details:, dependency: nil) client.record_update_job_error(error_type: error_type, error_details: error_details) end + sig do + params( + warn_type: T.any(String, Symbol), + warn_title: String, + warn_description: String + ).void + end + def record_update_job_warning(warn_type:, warn_title:, warn_description:) + client.record_update_job_warning( + warn_type: warn_type, + warn_title: warn_title, + warn_description: warn_description + ) + end + sig { params(error_type: T.any(String, Symbol), error_details: T.nilable(T::Hash[T.untyped, T.untyped])).void } def record_update_job_unknown_error(error_type:, error_details:) client.record_update_job_unknown_error(error_type: error_type, error_details: error_details) diff --git a/updater/lib/dependabot/updater/group_update_creation.rb b/updater/lib/dependabot/updater/group_update_creation.rb index 202f2178658..abf23f6492f 100644 --- a/updater/lib/dependabot/updater/group_update_creation.rb +++ b/updater/lib/dependabot/updater/group_update_creation.rb @@ -24,6 +24,7 @@ class Updater module GroupUpdateCreation extend T::Sig extend T::Helpers + include PullRequestHelpers abstract! @@ -122,6 +123,10 @@ def compile_all_dependency_changes_for(group) return nil end + # Send warning alerts to the API if any warning notices are present. + # Note that only notices with notice.show_alert set to true will be sent. + record_warning_notices(notices) if notices.any? + dependency_change ensure cleanup_workspace diff --git a/updater/lib/dependabot/updater/operations/create_security_update_pull_request.rb b/updater/lib/dependabot/updater/operations/create_security_update_pull_request.rb index 24568df99a5..156a48f3b36 100644 --- a/updater/lib/dependabot/updater/operations/create_security_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/create_security_update_pull_request.rb @@ -13,6 +13,7 @@ module Operations class CreateSecurityUpdatePullRequest extend T::Sig include SecurityUpdateHelpers + include PullRequestHelpers sig { params(job: Job).returns(T::Boolean) } def self.applies_to?(job:) @@ -180,9 +181,14 @@ def check_and_create_pull_request(dependency) dependency_files: dependency_snapshot.dependency_files, updated_dependencies: updated_deps, change_source: checker.dependency, + # Sending notices to the pr message builder to be used in the PR message if show_in_pr is true notices: @notices ) + # Send warning alerts to the API if any warning notices are present. + # Note that only notices with notice.show_alert set to true will be sent. + record_warning_notices(notices) if notices.any? + create_pull_request(dependency_change) rescue Dependabot::AllVersionsIgnored Dependabot.logger.info("All updates for #{dependency.name} were ignored") diff --git a/updater/lib/dependabot/updater/operations/refresh_security_update_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_security_update_pull_request.rb index a818c8dfb92..a70b466217c 100644 --- a/updater/lib/dependabot/updater/operations/refresh_security_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_security_update_pull_request.rb @@ -19,6 +19,7 @@ module Operations class RefreshSecurityUpdatePullRequest extend T::Sig include SecurityUpdateHelpers + include PullRequestHelpers sig { params(job: Job).returns(T::Boolean) } def self.applies_to?(job:) @@ -158,9 +159,14 @@ def check_and_update_pull_request(dependencies) dependency_files: dependency_snapshot.dependency_files, updated_dependencies: updated_deps, change_source: checker.dependency, + # Sending notices to the pr message builder to be used in the PR message if show_in_pr is true notices: @notices ) + # Send warning alerts to the API if any warning notices are present. + # Note that only notices with notice.show_alert set to true will be sent. + record_warning_notices(notices) if notices.any? + # NOTE: Gradle, Maven and Nuget dependency names can be case-insensitive # and the dependency name in the security advisory often doesn't match # what users have specified in their manifest. diff --git a/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb index 747926e940b..aafa2b2e13f 100644 --- a/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb @@ -16,6 +16,7 @@ class Updater module Operations class RefreshVersionUpdatePullRequest extend T::Sig + include PullRequestHelpers sig { params(job: Dependabot::Job).returns(T::Boolean) } def self.applies_to?(job:) @@ -146,9 +147,14 @@ def check_and_update_pull_request(dependencies) dependency_files: dependency_snapshot.dependency_files, updated_dependencies: updated_deps, change_source: checker.dependency, + # Sending notices to the pr message builder to be used in the PR message if show_in_pr is true notices: @notices ) + # Send warning alerts to the API if any warning notices are present. + # Note that only notices with notice.show_alert set to true will be sent. + record_warning_notices(notices) if notices.any? + # NOTE: Gradle, Maven and Nuget dependency names can be case-insensitive # and the dependency name in the security advisory often doesn't match # what users have specified in their manifest. diff --git a/updater/lib/dependabot/updater/operations/update_all_versions.rb b/updater/lib/dependabot/updater/operations/update_all_versions.rb index 926a81e6ec0..bdcc9a20c4b 100644 --- a/updater/lib/dependabot/updater/operations/update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/update_all_versions.rb @@ -12,6 +12,7 @@ class Updater module Operations class UpdateAllVersions extend T::Sig + include PullRequestHelpers sig { params(_job: Dependabot::Job).returns(T::Boolean) } def self.applies_to?(_job:) @@ -167,6 +168,7 @@ def check_and_create_pull_request(dependency) dependency_files: dependency_snapshot.dependency_files, updated_dependencies: updated_deps, change_source: checker.dependency, + # Sending notices to the pr message builder to be used in the PR message if show_in_pr is true notices: @notices ) @@ -174,6 +176,10 @@ def check_and_create_pull_request(dependency) raise "UpdateChecker found viable dependencies to be updated, but FileUpdater failed to update any files" end + # Send warning alerts to the API if any warning notices are present. + # Note that only notices with notice.show_alert set to true will be sent. + record_warning_notices(notices) if notices.any? + create_pull_request(dependency_change) end # rubocop:enable Metrics/PerceivedComplexity diff --git a/updater/lib/dependabot/updater/security_update_helpers.rb b/updater/lib/dependabot/updater/security_update_helpers.rb index 558a88234c8..e4827eea431 100644 --- a/updater/lib/dependabot/updater/security_update_helpers.rb +++ b/updater/lib/dependabot/updater/security_update_helpers.rb @@ -181,5 +181,52 @@ def security_update_not_possible_message(checker, latest_allowed_version, confli end end end + + module PullRequestHelpers + extend T::Sig + extend T::Helpers + + sig { returns(Dependabot::Service) } + attr_reader :service + + abstract! + + sig { params(notices: T.nilable(T::Array[Dependabot::Notice])).void } + def record_warning_notices(notices) + return if !notices || notices.empty? + + # Find unique warning notices which are going to be shown on insight page. + warn_notices = unique_warn_notices(notices) + + warn_notices.each do |notice| + # If alert is enabled, sending the deprecation notice to the service for showing on the UI insight page + send_alert_notice(notice) if notice.show_alert + end + rescue StandardError => e + Dependabot.logger.error( + "Failed to send notice warning: #{e.message}" + ) + end + + private + + # Resurns unique warning notices which are going to be shown on insight page. + sig { params(notices: T::Array[Dependabot::Notice]).returns(T::Array[Dependabot::Notice]) } + def unique_warn_notices(notices) + notices + .select { |notice| notice.mode == Dependabot::Notice::NoticeMode::WARN } + .uniq { |notice| [notice.type, notice.package_manager_name] } + end + + sig { params(notice: Dependabot::Notice).void } + def send_alert_notice(notice) + # Sending the notice to the service for showing on the dependabot insight page + service.record_update_job_warning( + warn_type: notice.type, + warn_title: notice.title, + warn_description: notice.description + ) + end + end end end diff --git a/updater/spec/dependabot/api_client_spec.rb b/updater/spec/dependabot/api_client_spec.rb index f40c622957e..36779ba6fc2 100644 --- a/updater/spec/dependabot/api_client_spec.rb +++ b/updater/spec/dependabot/api_client_spec.rb @@ -379,6 +379,47 @@ end end + describe "record_update_job_warning" do + let(:record_update_job_warning_url) { "http://example.com/update_jobs/1/record_update_job_warning" } + + let(:warn_type) { "test_warning_type" } + let(:warn_title) { "Test Warning Title" } + let(:warn_description) { "Test Warning Description" } + + before do + stub_request(:post, record_update_job_warning_url) + .to_return(status: 204, headers: headers) + end + + it "hits the correct endpoint" do + client.record_update_job_warning( + warn_type: warn_type, + warn_title: warn_title, + warn_description: warn_description + ) + + expect(WebMock) + .to have_requested(:post, record_update_job_warning_url) + .with(headers: { "Authorization" => "token" }) + end + + it "encodes the payload correctly" do + client.record_update_job_warning( + warn_type: warn_type, + warn_title: warn_title, + warn_description: warn_description + ) + + expect(WebMock).to(have_requested(:post, record_update_job_warning_url).with do |req| + data = JSON.parse(req.body)["data"] + + expect(data["warn-type"]).to eq(warn_type) + expect(data["warn-title"]).to eq(warn_title) + expect(data["warn-description"]).to eq(warn_description) + end) + end + end + describe "mark_job_as_processed" do let(:url) { "http://example.com/update_jobs/1/mark_as_processed" } let(:base_commit) { "sha" } diff --git a/updater/spec/dependabot/service_spec.rb b/updater/spec/dependabot/service_spec.rb index a0f9572d71b..2af5f2a5839 100644 --- a/updater/spec/dependabot/service_spec.rb +++ b/updater/spec/dependabot/service_spec.rb @@ -23,7 +23,8 @@ update_pull_request: nil, close_pull_request: nil, record_update_job_error: nil, - record_update_job_unknown_error: nil + record_update_job_unknown_error: nil, + record_update_job_warning: nil }) allow(api_client).to receive(:is_a?).with(Dependabot::ApiClient).and_return(true) api_client @@ -305,6 +306,28 @@ end end + describe "#record_update_job_warning" do + let(:warn_type) { :deprecated_dependency } + let(:warn_title) { "Deprecated Dependency Used" } + let(:warn_description) { "The dependency xyz is deprecated and should be updated or removed." } + + before do + service.record_update_job_warning( + warn_type: warn_type, + warn_title: warn_title, + warn_description: warn_description + ) + end + + it "delegates to @client" do + expect(mock_client).to have_received(:record_update_job_warning).with( + warn_type: warn_type, + warn_title: warn_title, + warn_description: warn_description + ) + end + end + describe "#capture_exception" do before do allow(Dependabot::Experiments).to receive(:enabled?).with(:record_update_job_unknown_error).and_return(true) diff --git a/updater/spec/dependabot/updater/operations/create_group_update_pull_request_spec.rb b/updater/spec/dependabot/updater/operations/create_group_update_pull_request_spec.rb index 0f19a7a515f..9f7cf5ac8ba 100644 --- a/updater/spec/dependabot/updater/operations/create_group_update_pull_request_spec.rb +++ b/updater/spec/dependabot/updater/operations/create_group_update_pull_request_spec.rb @@ -110,6 +110,19 @@ class_double(Dependabot::Bundler::UpdateChecker, new: stub_update_checker) end + let(:warning_deprecation_notice) do + Dependabot::Notice.new( + mode: "WARN", + type: "bundler_deprecated_warn", + package_manager_name: "bundler", + title: "Package manager deprecation notice", + description: "Dependabot will stop supporting `bundler v1`!\n" \ + "\n\nPlease upgrade to one of the following versions: `v2`, or `v3`.\n", + show_in_pr: true, + show_alert: true + ) + end + let(:stub_dependency_change) do instance_double( Dependabot::DependencyChange, @@ -117,18 +130,7 @@ should_replace_existing_pr?: false, grouped_update?: false, matches_existing_pr?: false, - notices: [ - Dependabot::Notice.new( - mode: "WARN", - type: "bundler_deprecated_warn", - package_manager_name: "bundler", - title: "Package manager deprecation notice", - description: "Dependabot will stop supporting `bundler v1`!\n" \ - "\n\nPlease upgrade to one of the following versions: `v2`, or `v3`.\n", - show_in_pr: true, - show_alert: true - ).to_hash - ] + notices: [warning_deprecation_notice] ) end @@ -159,18 +161,9 @@ context "when pull request does not already exist" do it "creates a pull request with deprecation notice" do expect(create_group_update_pull_request).to receive(:perform) - expect(stub_dependency_change.notices).to include( - { - mode: "WARN", - type: "bundler_deprecated_warn", - package_manager_name: "bundler", - title: "Package manager deprecation notice", - description: "Dependabot will stop supporting `bundler v1`!\n" \ - "\n\nPlease upgrade to one of the following versions: `v2`, or `v3`.\n", - show_in_pr: true, - show_alert: true - } - ) + expect(stub_dependency_change.notices) + .to include(warning_deprecation_notice) + create_group_update_pull_request.perform end end diff --git a/updater/spec/dependabot/updater/operations/create_security_update_pull_request_spec.rb b/updater/spec/dependabot/updater/operations/create_security_update_pull_request_spec.rb index ab7e1c0e251..8f798a9b207 100644 --- a/updater/spec/dependabot/updater/operations/create_security_update_pull_request_spec.rb +++ b/updater/spec/dependabot/updater/operations/create_security_update_pull_request_spec.rb @@ -15,6 +15,23 @@ require "dependabot/bundler" +# Stub PackageManagerBase +class StubPackageManager < Dependabot::PackageManagerBase + def initialize(name:, version:, deprecated_versions: [], unsupported_versions: [], supported_versions: []) + @name = name + @version = version + @deprecated_versions = deprecated_versions + @unsupported_versions = unsupported_versions + @supported_versions = supported_versions + end + + attr_reader :name + attr_reader :version + attr_reader :deprecated_versions + attr_reader :unsupported_versions + attr_reader :supported_versions +end + RSpec.describe Dependabot::Updater::Operations::CreateSecurityUpdatePullRequest do include DependencyFileHelpers include DummyPkgHelpers @@ -31,8 +48,15 @@ end let(:mock_service) do - instance_double(Dependabot::Service, increment_metric: nil, record_update_job_error: nil, create_pull_request: nil) + instance_double( + Dependabot::Service, + increment_metric: nil, + record_update_job_error: nil, + create_pull_request: nil, + record_update_job_warning: nil + ) end + let(:mock_error_handler) { instance_double(Dependabot::Updater::ErrorHandler) } let(:job_definition) do @@ -53,6 +77,19 @@ ) end + let(:package_manager) do + StubPackageManager.new( + name: "bundler", + version: package_manager_version, + deprecated_versions: deprecated_versions, + supported_versions: supported_versions + ) + end + + let(:package_manager_version) { "2" } + let(:supported_versions) { %w(2 3) } + let(:deprecated_versions) { %w(1) } + let(:job_definition_with_fetched_files) do job_definition.merge({ "base_commit_sha" => "mock-sha", @@ -157,70 +194,56 @@ end let(:stub_update_checker_class) do - Class.new do - define_method(:new) do |*_args| - stub_update_checker - end - end + class_double( + Dependabot::Bundler::UpdateChecker, + new: stub_update_checker + ) end - let(:transitive_stub_update_checker_class) do - Class.new do - define_method(:new) do |*_args| - transitive_stub_update_checker - end - end + let(:stub_dependency_change) do + instance_double( + Dependabot::DependencyChange, + updated_dependencies: [dependency], + updated_dependency_files: dependency_files, + should_replace_existing_pr?: false, + grouped_update?: false, + matches_existing_pr?: false + ) end - let(:concrete_package_manager_class) do - Class.new(Dependabot::PackageManagerBase) do - def name - "bundler" - end - - def version - Dependabot::Version.new("1.0.0") - end - - def deprecated_versions - [Dependabot::Version.new("1.0.0")] - end - - def unsupported_versions - [Dependabot::Version.new("0.9.0")] - end - - def supported_versions - [Dependabot::Version.new("1.1.0"), Dependabot::Version.new("2.0.0")] - end - - def support_later_versions? - true - end - end + let(:warning_deprecation_notice) do + Dependabot::Notice.new( + mode: "WARN", + type: "bundler_deprecated_warn", + package_manager_name: "bundler", + title: "Package manager deprecation notice", + description: "Dependabot will stop supporting `bundler v1`!\n" \ + "\n\nPlease upgrade to one of the following versions: `v2`, or `v3`.\n", + show_in_pr: true, + show_alert: true + ) end - let(:mock_package_manager_instance) { concrete_package_manager_class.new } - before do - allow(Dependabot::Experiments).to receive(:enabled?).with(:add_deprecation_warn_to_pr_message).and_return(true) - - # Allow for_package_manager to return the stub_update_checker_class - allow(Dependabot::UpdateCheckers).to receive(:for_package_manager).and_return(stub_update_checker_class) - - # Mock the DependencyChangeBuilder.create_from method - allow(Dependabot::DependencyChangeBuilder) - .to receive(:create_from) - .and_return(instance_double(Dependabot::DependencyChange)) - - # Mock the create_pull_request method - allow(create_security_update_pull_request) - .to receive(:create_pull_request) - - # Mock the package_manager method in dependency_snapshot - allow(dependency_snapshot) - .to receive(:package_manager) - .and_return(mock_package_manager_instance) + allow(Dependabot::Experiments) + .to receive(:enabled?) + .with(:add_deprecation_warn_to_pr_message) + .and_return(true) + + allow(Dependabot::UpdateCheckers).to receive( + :for_package_manager + ).and_return(stub_update_checker_class) + + allow(Dependabot::DependencyChangeBuilder).to receive( + :create_from + ).and_return(stub_dependency_change) + + allow(dependency_snapshot).to receive_messages( + job_dependencies: [dependency], + package_manager: package_manager, + notices: [warning_deprecation_notice] + ) + allow(job).to receive(:security_fix?).and_return(true) end after do @@ -228,60 +251,76 @@ def support_later_versions? end describe "#perform" do - context "when target_dependencies is empty" do + before do + allow(dependency_snapshot).to receive( + :dependencies + ).and_return([dependency]) + allow(job).to receive(:package_manager).and_return("bundler") + end + + context "when an error occurs" do + let(:error) { StandardError.new("error") } + before do - allow(dependency_snapshot) - .to receive(:job_dependencies).and_return([]) + allow(create_security_update_pull_request).to receive( + :check_and_create_pull_request + ).and_raise(error) end - it "records that no dependencies were found" do - expect(create_security_update_pull_request) - .to receive(:record_security_update_dependency_not_found) + it "handles the error with the error handler" do + expect(mock_error_handler).to receive( + :handle_dependency_error + ).with(error: error, dependency: dependency) perform end end - context "when target_dependencies is not empty" do + context "when no error occurs" do before do - allow(dependency_snapshot) - .to receive(:job_dependencies).and_return([dependency]) + allow(create_security_update_pull_request).to receive( + :check_and_create_pull_request + ) end - it "checks and creates pull requests for each dependency" do - expect(create_security_update_pull_request) - .to receive(:check_and_create_pr_with_error_handling).with(dependency) + it "does not handle any error" do + expect(mock_error_handler).not_to receive( + :handle_dependency_error + ) perform end + end - it "calls check_and_create_pull_request with the dependency" do - allow(create_security_update_pull_request) - .to receive(:check_and_create_pr_with_error_handling).and_call_original - expect(create_security_update_pull_request) - .to receive(:check_and_create_pull_request).with(dependency) - perform - end + context "when package manager version is deprecated" do + let(:package_manager_version) { "1" } - it "handles Dependabot::InconsistentRegistryResponse" do - allow(create_security_update_pull_request) - .to receive(:check_and_create_pull_request).and_raise(Dependabot::InconsistentRegistryResponse) - expect(mock_error_handler) - .to receive(:log_dependency_error).with( - dependency: dependency, - error: instance_of(Dependabot::InconsistentRegistryResponse), - error_type: "inconsistent_registry_response", - error_detail: anything + it "creates a pull request" do + expect(create_security_update_pull_request) + .to receive(:check_and_create_pull_request) + .with(dependency).and_call_original + expect(mock_service) + .to receive(:record_update_job_warning) + .with( + warn_type: warning_deprecation_notice.type, + warn_title: warning_deprecation_notice.title, + warn_description: warning_deprecation_notice.description ) + expect(create_security_update_pull_request) + .to receive(:create_pull_request) + .with(stub_dependency_change) perform end + end - it "handles StandardError" do - allow(create_security_update_pull_request) - .to receive(:check_and_create_pull_request).and_raise(StandardError) - expect(mock_error_handler) - .to receive(:handle_dependency_error).with( - error: instance_of(StandardError), - dependency: dependency - ) + context "when package manager version is not deprecated" do + let(:package_manager_version) { "2" } + + it "creates a pull request" do + expect(create_security_update_pull_request) + .to receive(:check_and_create_pull_request) + .with(dependency).and_call_original + expect(create_security_update_pull_request) + .to receive(:create_pull_request) + .with(stub_dependency_change) perform end end @@ -290,7 +329,8 @@ def support_later_versions? describe "#check_and_create_pull_request" do before do allow(create_security_update_pull_request) - .to receive(:update_checker_for).and_return(stub_update_checker) + .to receive(:update_checker_for) + .and_return(stub_update_checker) allow(dependency) .to receive(:all_versions).and_return(["4.0.0", "4.1.0", "4.2.0"]) end @@ -303,18 +343,26 @@ def support_later_versions? it "records that no update is needed if the version is correct" do allow(stub_update_checker.version_class) - .to receive(:correct?).with(dependency.version).and_return(true) + .to receive(:correct?) + .with(dependency.version) + .and_return(true) + expect(create_security_update_pull_request) - .to receive(:record_security_update_not_needed_error).with(dependency) - create_security_update_pull_request.send(:check_and_create_pull_request, dependency) + .to receive(:record_security_update_not_needed_error) + .with(dependency) + create_security_update_pull_request + .send(:check_and_create_pull_request, dependency) end it "records that the dependency file is not supported if the version is not correct" do allow(stub_update_checker.version_class) - .to receive(:correct?).with(dependency.version).and_return(false) + .to receive(:correct?) + .with(dependency.version).and_return(false) expect(create_security_update_pull_request) - .to receive(:record_dependency_file_not_supported_error).with(stub_update_checker) - create_security_update_pull_request.send(:check_and_create_pull_request, dependency) + .to receive(:record_dependency_file_not_supported_error) + .with(stub_update_checker) + create_security_update_pull_request + .send(:check_and_create_pull_request, dependency) end end @@ -342,8 +390,10 @@ def support_later_versions? it "checks if a pull request already exists" do expect(create_security_update_pull_request) - .to receive(:record_pull_request_exists_for_latest_version).with(stub_update_checker) - create_security_update_pull_request.send(:check_and_create_pull_request, dependency) + .to receive(:record_pull_request_exists_for_latest_version) + .with(stub_update_checker) + create_security_update_pull_request + .send(:check_and_create_pull_request, dependency) end context "when pull request doesn't exists" do @@ -354,32 +404,12 @@ def support_later_versions? ) end - it "creates a pull request without pr notices" do - expect(create_security_update_pull_request).to receive(:create_pull_request) - - create_security_update_pull_request.send(:check_and_create_pull_request, dependency) - end - - it "creates a pull request with pr notices" do - allow(Dependabot::Notice) - .to receive(:generate_pm_deprecation_notice) - .with(mock_package_manager_instance) - .and_return( - Dependabot::Notice.new( - mode: "WARN", - type: "bundler_deprecated_warn", - package_manager_name: "bundler", - title: "Package manager deprecation notice", - description: "Dependabot will stop supporting `bundler v1`!\n" \ - "\n\nPlease upgrade to one of the following versions: `v2`, or `v3`.\n", - show_in_pr: true, - show_alert: true - ) - ) - - expect(create_security_update_pull_request).to receive(:create_pull_request) + it "creates a pull request" do + expect(create_security_update_pull_request) + .to receive(:create_pull_request) - create_security_update_pull_request.send(:check_and_create_pull_request, dependency) + create_security_update_pull_request + .send(:check_and_create_pull_request, dependency) end end end @@ -387,22 +417,28 @@ def support_later_versions? context "when the update is not allowed" do before do allow(stub_update_checker) - .to receive(:requirements_unlocked_or_can_be?).and_return(false) + .to receive(:requirements_unlocked_or_can_be?) + .and_return(false) allow(stub_update_checker) - .to receive(:can_update?).with(requirements_to_unlock: :none).and_return(false) + .to receive(:can_update?) + .with(requirements_to_unlock: :none) + .and_return(false) end it "records that the update is not possible" do expect(create_security_update_pull_request) - .to receive(:record_security_update_not_possible_error).with(stub_update_checker) - create_security_update_pull_request.send(:check_and_create_pull_request, dependency) + .to receive(:record_security_update_not_possible_error) + .with(stub_update_checker) + create_security_update_pull_request + .send(:check_and_create_pull_request, dependency) end end context "when a transitive dependency conflict occurs" do before do allow(create_security_update_pull_request) - .to receive(:update_checker_for).and_return(transitive_stub_update_checker) + .to receive(:update_checker_for) + .and_return(transitive_stub_update_checker) allow(dependency_with_transitive_dependency) .to receive(:all_versions).and_return(["5.51.2", "2.0.1", "2.3.0"]) allow(job) @@ -411,12 +447,14 @@ def support_later_versions? it "records the conflict and returns from the function" do transitive_stub_update_checker = instance_double(Dependabot::UpdateCheckers::Base) - allow(transitive_stub_update_checker).to receive(:conflicting_dependencies).and_return([{ - "explanation" => "dummy-pkg-b@0.2.0 requires dummy-pkg-a@~2.0.1", - "name" => "dummy-pkg-b", - "version" => "0.2.0", - "requirement" => "~2.0.1" - }]) + allow(transitive_stub_update_checker) + .to receive(:conflicting_dependencies) + .and_return([{ + "explanation" => "dummy-pkg-b@0.2.0 requires dummy-pkg-a@~2.0.1", + "name" => "dummy-pkg-b", + "version" => "0.2.0", + "requirement" => "~2.0.1" + }]) allow(create_security_update_pull_request) .to receive(:check_and_create_pull_request).and_call_original @@ -426,7 +464,8 @@ def support_later_versions? expect(create_security_update_pull_request) .to receive(:record_security_update_not_possible_error) - create_security_update_pull_request.send(:check_and_create_pull_request, dependency_with_transitive_dependency) + create_security_update_pull_request + .send(:check_and_create_pull_request, dependency_with_transitive_dependency) end it "does not create a pull request if there is a conflict" do @@ -438,7 +477,8 @@ def support_later_versions? "requirement" => "~2.0.1" }]) expect(mock_service).not_to receive(:create_pull_request) - create_security_update_pull_request.send(:check_and_create_pull_request, transitive_dependency) + create_security_update_pull_request + .send(:check_and_create_pull_request, transitive_dependency) end end @@ -452,7 +492,8 @@ def support_later_versions? expect(Dependabot.logger) .to receive(:info).with("All updates for dummy-pkg-a were ignored") expect do - create_security_update_pull_request.send(:check_and_create_pull_request, dependency) + create_security_update_pull_request + .send(:check_and_create_pull_request, dependency) end.to raise_error(Dependabot::AllVersionsIgnored) end end diff --git a/updater/spec/dependabot/updater/operations/update_all_versions_spec.rb b/updater/spec/dependabot/updater/operations/update_all_versions_spec.rb index e98ff47238b..063ce77af5f 100644 --- a/updater/spec/dependabot/updater/operations/update_all_versions_spec.rb +++ b/updater/spec/dependabot/updater/operations/update_all_versions_spec.rb @@ -53,8 +53,10 @@ def initialize(name:, version:, deprecated_versions: [], unsupported_versions: [ let(:mock_service) do instance_double( Dependabot::Service, + increment_metric: nil, create_pull_request: nil, - close_pull_request: nil + close_pull_request: nil, + record_update_job_warning: nil ) end @@ -152,6 +154,19 @@ def initialize(name:, version:, deprecated_versions: [], unsupported_versions: [ ) end + let(:warning_deprecation_notice) do + Dependabot::Notice.new( + mode: "WARN", + type: "bundler_deprecated_warn", + package_manager_name: "bundler", + title: "Package manager deprecation notice", + description: "Dependabot will stop supporting `bundler v1`!\n" \ + "\n\nPlease upgrade to one of the following versions: `v2`, or `v3`.\n", + show_in_pr: true, + show_alert: true + ) + end + before do allow(Dependabot::Experiments).to receive(:enabled?).with(:add_deprecation_warn_to_pr_message).and_return(true) @@ -161,7 +176,9 @@ def initialize(name:, version:, deprecated_versions: [], unsupported_versions: [ allow(Dependabot::DependencyChangeBuilder).to receive( :create_from ).and_return(stub_dependency_change) - allow(dependency_snapshot).to receive_messages(package_manager: package_manager, notices: []) + allow(dependency_snapshot).to receive_messages(package_manager: package_manager, notices: [ + warning_deprecation_notice + ]) end after do @@ -213,6 +230,11 @@ def initialize(name:, version:, deprecated_versions: [], unsupported_versions: [ it "creates a pull request" do expect(update_all_versions).to receive(:check_and_create_pull_request).with(dependency).and_call_original + expect(mock_service).to receive(:record_update_job_warning).with( + warn_type: warning_deprecation_notice.type, + warn_title: warning_deprecation_notice.title, + warn_description: warning_deprecation_notice.description + ) expect(update_all_versions).to receive(:create_pull_request).with(stub_dependency_change) perform end diff --git a/updater/spec/dependabot/updater/pull_request_helpers_spec.rb b/updater/spec/dependabot/updater/pull_request_helpers_spec.rb new file mode 100644 index 00000000000..ed3f9f0fa8f --- /dev/null +++ b/updater/spec/dependabot/updater/pull_request_helpers_spec.rb @@ -0,0 +1,91 @@ +# typed: false +# frozen_string_literal: true + +require "spec_helper" +require "dependabot/updater" +require "dependabot/package_manager" +require "dependabot/notices" +require "dependabot/service" + +RSpec.describe Dependabot::Updater::PullRequestHelpers do + let(:dummy_class) do + Class.new do + include Dependabot::Updater::PullRequestHelpers + + attr_accessor :notices, :service + + def initialize(service = nil) + @notices = [] + @service = service + end + end + end + + let(:dummy_instance) { dummy_class.new(service) } + + let(:service) { instance_double(Dependabot::Service) } + + let(:package_manager) do + Class.new(Dependabot::PackageManagerBase) do + def name + "bundler" + end + + def version + Dependabot::Version.new("1") + end + + def deprecated_versions + [Dependabot::Version.new("1")] + end + + def supported_versions + [Dependabot::Version.new("2"), Dependabot::Version.new("3")] + end + end.new + end + + before do + allow(Dependabot::Experiments).to receive(:enabled?).with(:add_deprecation_warn_to_pr_message).and_return(true) + allow(service).to receive(:record_update_job_warning) + end + + after do + Dependabot::Experiments.reset! + end + + describe "#record_warning_notices" do + context "when deprecation notice is generated" do + let(:deprecation_notice) do + Dependabot::Notice.new( + mode: "WARN", + type: "bundler_deprecated_warn", + package_manager_name: "bundler", + title: "Package manager deprecation notice", + description: "Dependabot will stop supporting `bundler v1`!\n" \ + "\n\nPlease upgrade to one of the following versions: `v2`, or `v3`.\n", + show_in_pr: true, + show_alert: true + ) + end + + it "records it as a warning" do + expect(service).to receive(:record_update_job_warning).with( + warn_type: deprecation_notice.type, + warn_title: deprecation_notice.title, + warn_description: deprecation_notice.description + ) + + dummy_instance.record_warning_notices([deprecation_notice]) + end + end + + context "when no deprecation notice is generated" do + it "does not log or record any warnings" do + expect(service).not_to receive(:record_update_job_warning) + + dummy_instance.record_warning_notices([]) + end + end + end +end