Conversation
landongrindheim
left a comment
landongrindheim
left a comment
There was a problem hiding this comment.
🥇 Again, I'm loving the direction in which you're pushing our components to work together 😍
| extra: extra.merge({ | ||
| update_job_id: job&.id, | ||
| package_manager: job&.package_manager, | ||
| dependency_name: dependency&.name | ||
| }.compact) |
There was a problem hiding this comment.
🔎 I understand the impulse to send only those key/value pairs for which we have data, but I wonder if the absence of values is valuable for understanding context.
As a 🤏 cheap argument, I'd be able to glean much more insight from:
{
update_job_id: 1,
dependency_name: "dependabot-fortran",
package_manager: nil
}than
{
update_job_id: 1,
dependency_name: "dependabot-fortran"
}There was a problem hiding this comment.
That's fair, I think we should actually expect dependency_name to be passed/present move often -but- I'm err'ing on the side of maintaining existing behaviour for now so I'd prefer to pass on this to avoid test churn for now.
| @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? |
| Raven.capture_exception(error, raven_context) | ||
| service.capture_exception(error: error, job: job) |
There was a problem hiding this comment.
📈 This is such a nice improvement!
| 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), |
There was a problem hiding this comment.
🔎 I see we weren't handling this before, so no need to change anything. Should we provide a fallback value for Hash#fetch here?
There was a problem hiding this comment.
I think we probably want it to fail loud as we, ideally, should never enter this method unless we already checked the subject error was present in RUN_HALTING_ERRORS to begin with.
This code might be able to fold up into the call site in another pass? I don't think this is actually reused.
| # 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 |
There was a problem hiding this comment.
This feels like the right place for this 👍
2 participants
This is another clear-the-way PR for #6663 [Prototype] Generating grouped update PRs.
Previous PR: #6810
The
Dependabot::Updaterclass implements quite a lot of private methods to assist error handling. As we start to break the updater out into individual strategy classes which handle discrete operations, many of them need to share these methods.This PR clears the way to start breaking the logic apart primarily via three changes:
Dependabot::Updater. This isn't used in any significant way, we can just useDependabot::Service#errorsas a substitute in the one place we need to check if any errors have happened.service. record_update_job_errordirectly instead of creating an "error_detail" hash to use theDependabot::Updater#record_errorprivate method, which has been removedDependabot::Serviceclass so it has an explicit receiver that binds it to the backend service and acceptsDependabotobjects as argumentsThis will allow each individual strategy class to be passed the service so the various
record_foo_errorhelper private methods can be moved without dragging more duplicate private methods in.Other Changes
There are a few small tidy ups made alongside these changes:
error_contextmethod onDependabot::UpdaterDependabot::Service, notably disallow using itsDependabot::ApiClientdirectly