Conversation
landongrindheim
approved these changes
Mar 10, 2023
Contributor
landongrindheim
left a comment
landongrindheim
left a comment
There was a problem hiding this comment.
I love the direction these changes are moving us toward! 👨🍳👌
Comment on lines
88
to
89
Contributor
There was a problem hiding this comment.
Since a job is supposed to live/execute in a transient environment, I'm really digging Environment as a global (hopefully unchanging) space to store facts 😄
Contributor
Author
There was a problem hiding this comment.
Yeah, it is currently an Updater-isim but I think we should probably split it out into Core and have an Dependabot::UpdaterEnvironment module which extends Dependabot::Environment at some point as I like the pattern.
updater/lib/dependabot/job.rb
Outdated
Contributor
There was a problem hiding this comment.
👏 This is a great improvement!
brrygrdn
force-pushed
the
brrygrdn/harden-job-objects
branch
from
2688ec4 to
0e98ad8
Compare
This method was used in Dependabot Preview to retrieve the Job from an API call, in modern Dependabot the Job definition is always injected into the container by the Docker co-ordination layer ( the CLI, the Dependabot Action, etc ). This method doesn't need to be maintained anymore and it duplicates Job instantiation which presents a risk where we could naievely provide the wrong credentials to the Fetch/Update steps
Co-authored-by: Landon Grindheim <landon.grindheim@gmail.com>
brrygrdn
force-pushed
the
brrygrdn/harden-job-objects
branch
from
f793484 to
5490d47
Compare
This was referenced Mar 15, 2023
brrygrdn
added a commit
that referenced
this pull request
Mar 28, 2023
This should have been removed in #6810 Our API returns a 4xx response and a blank job description if a job that has already been processed is requested, but this no longer happens within the Dependabot::Updater code. The layer that constructs the docker containers which the code runs in will not be able to inject a file and will bail out before ever invoking any Ruby
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows up on #6770, #6792 and #6808
This is another clear-the-way PR for #6663 [Prototype] Generating grouped update PRs.
The primary changes in this PR are:
repo_contents_pathinto Dependabot::Job so we aren't mixing methods of referring back toDependabot::Environmentor passing it as an additional argumentThe aim of this is to have as little repetition around creating a
Dependabot::Jobas possible as well as binding all inputs shared by theFileFetcherCommandandUpateFilesCommandto it.This leaves the
base_commit_shaanddependency_filesas floating parameters which are produced by theFileFetcherCommandand consumed by theUpateFilesCommand. I don't think it makes sense to bind these to Dependabot::Job as they are a separate concern for that reason. I may bind these into a value class in a subsequent PR if this makes sense.