-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Raise DependencyFileUnsupported error when API returns matching 400 error #11438
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
require "dependabot/job" | ||
require "dependabot/opentelemetry" | ||
require "sorbet-runtime" | ||
require "dependabot/errors" | ||
|
||
# Provides a client to access the internal Dependabot Service's API | ||
# | ||
|
@@ -18,10 +19,11 @@ | |
module Dependabot | ||
class ApiError < StandardError; end | ||
|
||
class ApiClient | ||
class ApiClient # rubocop:disable Metrics/ClassLength | ||
extend T::Sig | ||
|
||
MAX_REQUEST_RETRIES = 3 | ||
INVALID_REQUEST_MSG = /The request contains invalid or unauthorized changes/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you know why we do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To quote the API code, it's because we are trying to update GitHub Actions workflow files when our package manager is not github_actions or the dependency file is not one of the files specified in the It was added as a fix to prevent any potential security issues raised in issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Adding @honeyankit as a reviewer. |
||
|
||
sig { params(base_url: String, job_id: T.any(String, Integer), job_token: String).void } | ||
def initialize(base_url, job_id, job_token) | ||
|
@@ -41,7 +43,12 @@ def create_pull_request(dependency_change, base_commit_sha) | |
api_url = "#{base_url}/update_jobs/#{job_id}/create_pull_request" | ||
data = create_pull_request_data(dependency_change, base_commit_sha) | ||
response = http_client.post(api_url, json: { data: data }) | ||
raise ApiError, response.body if response.code >= 400 | ||
|
||
if response.code >= 400 && dependency_file_not_supported_error?(response.body.to_s) | ||
raise Dependabot::DependencyFileNotSupported, response.body.to_s | ||
elsif response.code >= 400 | ||
raise ApiError, response.body | ||
end | ||
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError | ||
retry_count ||= 0 | ||
retry_count += 1 | ||
|
@@ -416,5 +423,15 @@ def create_pull_request_data(dependency_change, base_commit_sha) | |
data["pr-body"] = dependency_change.pr_message.pr_message | ||
data | ||
end | ||
|
||
sig { params(response: String).returns(T::Boolean) } | ||
def dependency_file_not_supported_error?(response) | ||
body = JSON.parse(response) | ||
|
||
return false unless body.is_a?(Hash) | ||
return false unless body["errors"] | ||
|
||
INVALID_REQUEST_MSG.match? body["errors"].first["detail"] | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, we could satisfy the cop if we extracted some logic out into it's own method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the class length so we would have to extract a mixin but then extracting a mixin just for this would probably not be worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should really make an effort to adhere by the suggestions from rubocop rather than disable them. Especially for a core class that is shared across ecosystems.