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

Jump to bottom

Added exception for security update error handling. #9977

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+30 −1
Open

Added exception for security update error handling. #9977

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
66e21c6
Added exception for security update error handling.
sachin-sandhu Jun 12, 2024
d658b26
Linting and fixes.
sachin-sandhu Jun 12, 2024
098fe4c
exception backtrace setup and error context assignment
sachin-sandhu Jun 13, 2024
6b80e43
Add exception to security update
sachin-sandhu Jun 18, 2024
d3ed7ea
Add exception to security update
sachin-sandhu Jun 18, 2024
28f3b30
Fixes formatting and tests
sachin-sandhu Jun 23, 2024
cdc9c91
Fixes dependabot file updater docker error.
sachin-sandhu Jun 24, 2024
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
18 changes: 16 additions & 2 deletions updater/lib/dependabot/updater/operations/create_security_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ module Operations
class CreateSecurityUpdatePullRequest
include SecurityUpdateHelpers

class SecurityUpdateError < StandardError
Copy link
Member

Choose a reason for hiding this comment

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

should we call this TransitiveSecurityUpdateError for specificity?

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 matched with class name convention, specificity would be a good idea for sure. ill wait for someone to review once to be sure in case im missing anything.

def initialize(message:, error_context:)
super(message)
@error_context = error_context
end

def sentry_context
{ extra: @error_context }
end
end

def self.applies_to?(job:)
return false if job.updating_a_pull_request?
# If we haven't been given data for the vulnerable dependency,
Expand Down Expand Up @@ -69,8 +80,11 @@ def check_and_create_pr_with_error_handling(dependency)
error_type: "inconsistent_registry_response",
Copy link
Member

Choose a reason for hiding this comment

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

I think the error is surfaced here.

can we detect the actual transitive failure mode and add a specific error class for that scenario? That way we are not swallowing up all errors.

I would expect something like this:

raise Dependabot::SecurityUpdateFailureDueToTransitiveDependency.new(...
...

That way, we can ensure we are actually separating out that error category.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdulapopoola , im working on this

error_detail: e.message
)
rescue StandardError => e
error_handler.handle_dependency_error(error: e, dependency: dependency)
rescue StandardError => standard_ex
security_ex = SecurityUpdateError.new(message: "Security Update Error, #{standard_ex.message}",
error_context: standard_ex.instance_variable_get(:@error_context))
security_ex.set_backtrace(standard_ex.backtrace)
error_handler.handle_dependency_error(error: security_ex, dependency: dependency)
end

# rubocop:disable Metrics/AbcSize
Expand Down
Loading