-
Notifications
You must be signed in to change notification settings - Fork 994
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
DependencyGroupChangeBatch tracks updated dependencies per file #9988
Conversation
f82a752
to
3f0ae6b
Compare
98aabc0
to
2edad0e
Compare
160f37f
to
471d5a5
Compare
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 this makes sense! I left a comment which I think should be addressed (how we read previous dependencies), but I think this is close to ready to go 😄
updater/lib/dependabot/updater/dependency_group_change_batch.rb
Outdated
Show resolved
Hide resolved
updater/lib/dependabot/updater/dependency_group_change_batch.rb
Outdated
Show resolved
Hide resolved
result = run_update_for(group) | ||
if result | ||
grouped_update_result = run_grouped_update_for(group) | ||
if grouped_update_result |
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.
📈
hsh[file.path] = { file: file, changed: false, changes: 0 } | ||
hsh[file.path] = { file: file, updated_dependencies: [], changed: false, changes: 0 } |
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.
Noting for the future: This seems like primitive obsession, and I think we'd be better off creating an object to handle these operations. I'd expect that to be easier to work with and less prone to making mistakes 🙂
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.
Thanks for noting that! I think that would be appropriate for the follow-up work of when we switch the DependencyGroupChangeBatch
over to read from the new updated_dependency_file[updated_dependencies]
field
# FIXME: All of the files in the dependency_file_batch will contain a copy of the updated dependencies. | ||
# Eventually this should be narrowed down so that only the files that were modified | ||
# by the newly merged dependency_change have their updated_dependencies updated. |
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.
🤔 What will the impact of this be in the short term?
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.
This field only exists in-memory so it's essentially thrown away when the DependencyGroupChangeBatch is dereferenced.
This PR sets the field up so it can be used in a follow-up PR.
dependency_file_batch = batch.instance_variable_get(:@dependency_file_batch) | ||
expect(dependency_file_batch["/Gemfile"][:updated_dependencies]).to eq(updated_dependencies) | ||
expect(dependency_file_batch["/Gemfile.lock"][:updated_dependencies]).to eq(updated_dependencies) | ||
expect(dependency_file_batch["/unknown/Gemfile"][:updated_dependencies]).to eq(updated_dependencies) |
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 appreciate the note here as I found the behavior unexpected. I do think it would make sense to expose a method if we need to be able to test this 🙂
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.
That's fair! I originally wanted to wait on that for the follow-up PR since the implementation of that method will probably change
I'll do it here though so it's clearer how it will be used in the future
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.
Actually, I think I will hold off on making this change!
It will be a modification of the current_dependency_files
method that gets called GroupUpdateCreation:
dependency_files = group_changes.current_dependency_files(job) |
The method and caller will need to be updated to return/accept the dependency files and read the list of updated_dependencies from them. It can then use that list to determine what the updated_dependency
is without having to scan through the global list of updated_dependencies like it currently does in #deduce_updated_dependency
:
dependency_snapshot.handled_dependencies << dependency.name |
8a16bc8
to
0a3e581
Compare
f83cc8a
to
39600d2
Compare
updated_dependencies
Co-authored-by: Landon Grindheim <landon.grindheim@gmail.com>
39600d2
to
8962ae7
Compare
8962ae7
to
dfe9c96
Compare
What are you trying to accomplish?
Requires #9982
Now that a
Dependabot::Dependency
can have a directory, we should use it to track which dependencies are updated in eachupdated_dependency_file
in aDependencyGroupChangeBatch
. This will allow us to keep track of every dependency update across every file.A
DependencyGroupChangeBatch
is used to hold and consolidate theDependencyChanges
when updating a group. For a multi-directory PR, theDependencyGroupChangeBatch
represents updating a group across every directory.Prior to this change, the
DependencyGroupChangeBatch
would overwrite theupdated_dependencies
from the previous directory. This meant that if a dependency was updated in both, we would only keep the changes from the later update.By tracking dependencies per file, we ensure we keep track of every dependency update across every file and don't miss any updates for a group.
Anything you want to highlight for special attention from reviewers?
This PR makes use of the feature flag
:dependency_has_directory
which is passed in to the job definition from the APIHow will you know you've accomplished your goal?
This PR only adds
updated_dependencies
to theupdated_dependency_files
but it is not yet used.A follow up PR that switches the DependencyGroupChangeBatch over to reading from the
updated_dependency_file[updated_dependencies]
field would affect PR creation and will be implemented in a follow-up. For now this PR should have no effect, even when the feature flag is toggled.We have a test repo where we can compare the changes of this PR before and after toggling the feature flag to confirm.
Checklist