diff --git a/updater/lib/dependabot/base_command.rb b/updater/lib/dependabot/base_command.rb index a11aa2e98b6..7222c251372 100644 --- a/updater/lib/dependabot/base_command.rb +++ b/updater/lib/dependabot/base_command.rb @@ -64,37 +64,24 @@ def handle_exception(err) Dependabot.logger.error(err.message) err.backtrace.each { |line| Dependabot.logger.error(line) } - Raven.capture_exception(err, raven_context) - + service.capture_exception(error: err, job: job) service.record_update_job_error(error_type: "unknown_error", error_details: { message: err.message }) end - def api_url - Environment.api_url - end - def job_id Environment.job_id end - def job_token - Environment.job_token - end - def api_client - @api_client ||= Dependabot::ApiClient.new(api_url, job_id, job_token) + @api_client ||= Dependabot::ApiClient.new( + Environment.api_url, + job_id, + Environment.job_token + ) end def service @service ||= Dependabot::Service.new(client: api_client) end - - private - - def raven_context - context = { tags: {}, extra: { update_job_id: job_id } } - context[:tags][:package_manager] = job.package_manager if job - context - end end end diff --git a/updater/lib/dependabot/file_fetcher_command.rb b/updater/lib/dependabot/file_fetcher_command.rb index 9601db9cb88..7e9c90c96b0 100644 --- a/updater/lib/dependabot/file_fetcher_command.rb +++ b/updater/lib/dependabot/file_fetcher_command.rb @@ -170,8 +170,8 @@ def handle_file_fetcher_error(error) else Dependabot.logger.error(error.message) error.backtrace.each { |line| Dependabot.logger.error line } - Raven.capture_exception(error, raven_context) + service.capture_exception(error: error, job: job) { "error-type": "unknown_error" } end diff --git a/updater/lib/dependabot/service.rb b/updater/lib/dependabot/service.rb index ab3a33d9acf..32fe79bbb50 100644 --- a/updater/lib/dependabot/service.rb +++ b/updater/lib/dependabot/service.rb @@ -3,13 +3,16 @@ require "terminal-table" require "dependabot/api_client" -# Wraps an API client with the current state of communications with the Dependabot Service -# and provides an interface to summarise all actions taken. +# This class provides an output adapter for the Dependabot Service which manages +# communication with the private API as well as consolidated error handling. +# +# Currently this is the only output adapter available, but in future we may +# support others for use with the dependabot/cli project. # module Dependabot class Service extend Forwardable - attr_reader :client, :events, :pull_requests, :errors + attr_reader :pull_requests, :errors def initialize(client:) @client = client @@ -39,6 +42,25 @@ 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 + # This method wraps the Raven client as the Application error tracker + # the service uses to notice errors. + # + # This should be called as an alternative/in addition to record_update_job_error + # for cases where an error could indicate a problem with the service. + def capture_exception(error:, job: nil, dependency: nil, tags: {}, extra: {}) + Raven.capture_exception( + error, + { + tags: tags, + extra: extra.merge({ + update_job_id: job&.id, + package_manager: job&.package_manager, + dependency_name: dependency&.name + }.compact) + } + ) + end + def noop? pull_requests.empty? && errors.empty? end @@ -71,6 +93,8 @@ def summary private + attr_reader :client + def pull_request_summary return unless pull_requests.any? diff --git a/updater/lib/dependabot/updater.rb b/updater/lib/dependabot/updater.rb index 72623600a5a..8190cd5dbd2 100644 --- a/updater/lib/dependabot/updater.rb +++ b/updater/lib/dependabot/updater.rb @@ -60,8 +60,7 @@ def initialize(service:, job:, dependency_files:, base_commit_sha:) @job = job @dependency_files = dependency_files @base_commit_sha = base_commit_sha - # TODO: Collect @created_pull_requests and @errors on the Job object? - @errors = [] + # TODO: Collect @created_pull_requests on the Job object? @created_pull_requests = [] end @@ -81,18 +80,20 @@ def run "Dependabot::AllVersionsIgnored was unexpectedly raised for a non-security update job" ) error.set_backtrace(e.backtrace) - Raven.capture_exception(error, raven_context) + service.capture_exception(error: error, job: job) return end # OOM errors are special cased so that we stop the update run early - error = { "error-type": RUN_HALTING_ERRORS.fetch(e.class) } - record_error(error) + service.record_update_job_error( + error_type: RUN_HALTING_ERRORS.fetch(e.class), + error_details: nil + ) end private - attr_accessor :errors, :created_pull_requests + attr_accessor :created_pull_requests attr_reader :service, :job, :dependency_files, :base_commit_sha def check_and_create_pr_with_error_handling(dependency) @@ -130,7 +131,16 @@ def check_and_update_existing_pr_with_error_handling(dependencies) # DependencyChange as we build it up step-by-step. def check_and_update_pull_request(dependencies) if dependencies.count != job.dependencies.count - close_pull_request(reason: :dependency_removed) unless errors.any? + # TODO: Remove the unless statement + # + # This check is to determine if there was an error parsing the dependency + # dependency file. + # + # For update existing PR operations we should early exit after a failed + # parse instead, but we currently share the `#dependencies` method + # with other code paths. This will be fixed as we break out Operation + # classes. + close_pull_request(reason: :dependency_removed) unless service.errors.any? return end @@ -329,12 +339,10 @@ def record_security_update_not_needed_error(checker) "is no longer vulnerable" ) - record_error( - { - "error-type": "security_update_not_needed", - "error-detail": { - "dependency-name": checker.dependency.name - } + service.record_update_job_error( + error_type: "security_update_not_needed", + error_details: { + "dependency-name": checker.dependency.name } ) end @@ -345,12 +353,10 @@ def record_security_update_ignored(checker) "were ignored for #{checker.dependency.name}" ) - record_error( - { - "error-type": "all_versions_ignored", - "error-detail": { - "dependency-name": checker.dependency.name - } + service.record_update_job_error( + error_type: "all_versions_ignored", + error_details: { + "dependency-name": checker.dependency.name } ) end @@ -362,12 +368,10 @@ def record_dependency_file_not_supported_error(checker) "installed version of #{checker.dependency.name} isn't known." ) - record_error( - { - "error-type": "dependency_file_not_supported", - "error-detail": { - "dependency-name": checker.dependency.name - } + service.record_update_job_error( + error_type: "dependency_file_not_supported", + error_details: { + "dependency-name": checker.dependency.name } ) end @@ -386,15 +390,13 @@ def record_security_update_not_possible_error(checker) ) Dependabot.logger.info(earliest_fixed_version_message(lowest_non_vulnerable_version)) - record_error( - { - "error-type": "security_update_not_possible", - "error-detail": { - "dependency-name": checker.dependency.name, - "latest-resolvable-version": latest_allowed_version, - "lowest-non-vulnerable-version": lowest_non_vulnerable_version, - "conflicting-dependencies": conflicting_dependencies - } + service.record_update_job_error( + error_type: "security_update_not_possible", + error_details: { + "dependency-name": checker.dependency.name, + "latest-resolvable-version": latest_allowed_version, + "lowest-non-vulnerable-version": lowest_non_vulnerable_version, + "conflicting-dependencies": conflicting_dependencies } ) end @@ -406,26 +408,22 @@ def record_security_update_not_found(checker) "The latest available version is #{checker.dependency.version}" ) - record_error( - { - "error-type": "security_update_not_found", - "error-detail": { - "dependency-name": checker.dependency.name, - "dependency-version": checker.dependency.version - } + service.record_update_job_error( + error_type: "security_update_not_found", + error_details: { + "dependency-name": checker.dependency.name, + "dependency-version": checker.dependency.version }, dependency: checker.dependency ) end def record_pull_request_exists_for_latest_version(checker) - record_error( - { - "error-type": "pull_request_exists_for_latest_version", - "error-detail": { - "dependency-name": checker.dependency.name, - "dependency-version": checker.latest_version&.to_s - } + service.record_update_job_error( + error_type: "pull_request_exists_for_latest_version", + error_details: { + "dependency-name": checker.dependency.name, + "dependency-version": checker.latest_version&.to_s }, dependency: checker.dependency ) @@ -439,12 +437,11 @@ def record_pull_request_exists_for_security_update(existing_pull_request) "dependency-removed": dep.fetch("dependency-removed", nil) }.compact end - record_error( - { - "error-type": "pull_request_exists_for_security_update", - "error-detail": { - "updated-dependencies": updated_dependencies - } + + service.record_update_job_error( + error_type: "pull_request_exists_for_security_update", + error_details: { + "updated-dependencies": updated_dependencies } ) end @@ -789,6 +786,7 @@ def close_pull_request(reason:) service.close_pull_request(job.dependencies, reason) end + # rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/MethodLength def handle_dependabot_error(error:, dependency:) error_details = @@ -859,7 +857,7 @@ def handle_dependabot_error(error:, dependency:) msg = "Subprocess #{error.raven_context[:fingerprint]} failed to run. Check the job logs for error messages" sanitized_error = SubprocessFailed.new(msg, raven_context: error.raven_context) sanitized_error.set_backtrace(error.backtrace) - Raven.capture_exception(sanitized_error, raven_context) + service.capture_exception(error: sanitized_error, job: job) { "error-type": "unknown_error" } when *Octokit::RATE_LIMITED_ERRORS @@ -872,11 +870,19 @@ def handle_dependabot_error(error:, dependency:) } } else - Raven.capture_exception(error, raven_context(dependency: dependency)) + service.capture_exception( + error: error, + job: job, + dependency: dependency + ) { "error-type": "unknown_error" } end - record_error(error_details, dependency: dependency) + service.record_update_job_error( + error_type: error_details.fetch(:"error-type"), + error_details: error_details[:"error-detail"], + dependency: dependency + ) log_error( dependency: dependency, @@ -885,11 +891,16 @@ def handle_dependabot_error(error:, dependency:) error_detail: error_details.fetch(:"error-detail", nil) ) end - # rubocop:enable Metrics/MethodLength + # rubocop:enable Metrics/AbcSize + # rubocop:disable Metrics/MethodLength - # rubocop:disable Metrics/CyclomaticComplexity def handle_parser_error(error) + # This happens if the repo gets removed after a job gets kicked off. + # The service will handle the removal without any prompt from the updater, + # so no need to add an error to the errors array + return if error.is_a? Dependabot::RepoNotFound + error_details = case error when Dependabot::DependencyFileNotEvaluatable @@ -907,11 +918,6 @@ def handle_parser_error(error) "error-type": "branch_not_found", "error-detail": { "branch-name": error.branch_name } } - when Dependabot::RepoNotFound - # This happens if the repo gets removed after a job gets kicked off. - # The main backend will handle it without any prompt from the updater, - # so no need to add an error to the errors array - nil when Dependabot::DependencyFileNotParseable { "error-type": "dependency_file_not_parseable", @@ -958,14 +964,16 @@ def handle_parser_error(error) Dependabot.logger.error error.message error.backtrace.each { |line| Dependabot.logger.error line } - Raven.capture_exception(error, raven_context) + service.capture_exception(error: error, job: job) { "error-type": "unknown_error" } end - record_error(error_details) if error_details + service.record_update_job_error( + error_type: error_details.fetch(:"error-type"), + error_details: error_details[:"error-detail"] + ) end # rubocop:enable Metrics/MethodLength - # rubocop:enable Metrics/CyclomaticComplexity def update_dependency_list(dependencies) service.update_dependency_list( @@ -979,32 +987,6 @@ def update_dependency_list(dependencies) dependency_files.reject(&:support_file).map(&:path) ) end - - def error_context(dependency) - { dependency_name: dependency.name, update_job_id: job.id } - end - - def record_error(error_details, dependency: nil) - service.record_update_job_error( - error_type: error_details.fetch(:"error-type"), - error_details: error_details[:"error-detail"], - dependency: dependency - ) - - errors << error_details - end - - def raven_context(dependency: nil) - context = { - tags: {}, - extra: { - update_job_id: job.id, - package_manager: job.package_manager - } - } - context[:extra][:dependency_name] = dependency.name if dependency - context - end end end # rubocop:enable Metrics/ClassLength diff --git a/updater/spec/dependabot/file_fetcher_command_spec.rb b/updater/spec/dependabot/file_fetcher_command_spec.rb index 00ecae53118..892039ce323 100644 --- a/updater/spec/dependabot/file_fetcher_command_spec.rb +++ b/updater/spec/dependabot/file_fetcher_command_spec.rb @@ -11,9 +11,9 @@ let(:job_id) { "123123" } before do - allow(job).to receive(:job_id).and_return(job_id) - allow(job).to receive(:job_token).and_return("job_token") - allow(job).to receive(:api_client).and_return(api_client) + allow(Dependabot::Environment).to receive(:job_id).and_return(job_id) + allow(Dependabot::Environment).to receive(:job_token).and_return("job_token") + allow(Dependabot::ApiClient).to receive(:new).and_return(api_client) allow(api_client).to receive(:mark_job_as_processed) allow(api_client).to receive(:record_update_job_error) diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index 973e887615d..74e5cf3331a 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/updater/spec/dependabot/updater_spec.rb @@ -155,7 +155,7 @@ updater.run expect(service).to have_received(:record_update_job_error). - with({ error_type: "out_of_disk", error_details: nil, dependency: nil }) + with({ error_type: "out_of_disk", error_details: nil }) end end @@ -295,8 +295,7 @@ error_type: "dependency_file_not_supported", error_details: { "dependency-name": "dummy-pkg-b" - }, - dependency: nil + } } ) expect(Dependabot.logger). @@ -380,8 +379,7 @@ "requirement" => "= 1.2.0" } ] - }, - dependency: nil + } } ) expect(Dependabot.logger). @@ -425,8 +423,7 @@ "latest-resolvable-version": "1.1.0", "lowest-non-vulnerable-version": nil, "conflicting-dependencies": [] - }, - dependency: nil + } } ) expect(Dependabot.logger). @@ -1016,8 +1013,7 @@ def expect_update_checker_with_ignored_versions(versions) "dependency-name": "dummy-pkg-b", "dependency-version": "1.2.0" ] - }, - dependency: nil + } ) expect(Dependabot.logger). to receive(:info). @@ -1167,8 +1163,7 @@ def expect_update_checker_with_ignored_versions(versions) "dependency-removed": true } ] - }, - dependency: nil + } ) expect(Dependabot.logger). to receive(:info). @@ -1300,6 +1295,11 @@ def expect_update_checker_with_ignored_versions(versions) allow(updater).to receive(:dependency_files). and_raise(Dependabot::DependencyFileNotParseable.new("path/to/file")) + expect(service).to receive(:record_update_job_error).with( + error_type: "dependency_file_not_parseable", + error_details: anything + ) + expect(service).to receive(:errors).and_return([anything]) expect(service).to_not receive(:close_pull_request) updater.run @@ -1549,8 +1549,7 @@ def expect_update_checker_with_ignored_versions(versions) error_type: "all_versions_ignored", error_details: { "dependency-name": "dummy-pkg-b" - }, - dependency: nil + } } ) expect(Dependabot.logger). @@ -1587,8 +1586,7 @@ def expect_update_checker_with_ignored_versions(versions) error_type: "security_update_not_needed", error_details: { "dependency-name": "dummy-pkg-b" - }, - dependency: nil + } } ) expect(Dependabot.logger). @@ -1638,8 +1636,7 @@ def expect_update_checker_with_ignored_versions(versions) to receive(:record_update_job_error). with( error_type: "unknown_error", - error_details: nil, - dependency: nil + error_details: nil ) updater.run @@ -1679,8 +1676,7 @@ def expect_update_checker_with_ignored_versions(versions) to receive(:record_update_job_error). with( error_type: "dependency_file_not_found", - error_details: { "file-path": "path/to/file" }, - dependency: nil + error_details: { "file-path": "path/to/file" } ) updater.run @@ -1721,8 +1717,7 @@ def expect_update_checker_with_ignored_versions(versions) to receive(:record_update_job_error). with( error_type: "branch_not_found", - error_details: { "branch-name": "my_branch" }, - dependency: nil + error_details: { "branch-name": "my_branch" } ) updater.run @@ -1763,8 +1758,7 @@ def expect_update_checker_with_ignored_versions(versions) to receive(:record_update_job_error). with( error_type: "dependency_file_not_parseable", - error_details: { "file-path": "path/to/file", message: "a" }, - dependency: nil + error_details: { "file-path": "path/to/file", message: "a" } ) updater.run @@ -1805,8 +1799,7 @@ def expect_update_checker_with_ignored_versions(versions) to receive(:record_update_job_error). with( error_type: "path_dependencies_not_reachable", - error_details: { dependencies: ["bad_gem"] }, - dependency: nil + error_details: { dependencies: ["bad_gem"] } ) updater.run @@ -2513,8 +2506,9 @@ def default_dependency_files end def build_service - instance_double( - Dependabot::Service, + # Stub out a client so we don't hit the internet + api_client = instance_double( + Dependabot::ApiClient, create_pull_request: nil, update_pull_request: nil, close_pull_request: nil, @@ -2522,6 +2516,13 @@ def build_service update_dependency_list: nil, record_update_job_error: nil ) + + service = Dependabot::Service.new( + client: api_client + ) + allow(service).to receive(:record_update_job_error) + + service end def build_job(requested_dependencies: nil, allowed_updates: default_allowed_updates, # rubocop:disable Metrics/MethodLength