[Updater] The Updater always expects job.dependency_groups to be defined #7216
Conversation
This file contains 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
This was referenced May 2, 2023
Merged
Nishnha
reviewed
May 3, 2023
Comment on lines
+78
to
+80
| # If no groups are defined, all dependencies are ungrouped by default. | ||
| return allowed_dependencies unless groups.any? | ||
|
|
Nishnha
reviewed
May 3, 2023
| branch: | ||
| api-endpoint: https://api.github.com/ | ||
| hostname: github.com | ||
| dependencies: |
There was a problem hiding this comment.
I think you would want to include dummy-pkg-a and dummy-pkg-b under the dependencies key here?
There was a problem hiding this comment.
Ah, good eye - for "All Version" updates, we ignore any content in this array and the backend won't actually populate it so I've started leaving it blank when some of our older test fixtures fill it in.
Nishnha
approved these changes
May 3, 2023
brrygrdn
force-pushed
the
brrygrdn/updater-does-not-inject-groups
branch
from
8020195
to
057d1ee
Compare
brettfo
pushed a commit
to brettfo/dependabot-core
that referenced
this pull request
Oct 11, 2023
…oes-not-inject-groups [Updater] The Updater always expects job.dependency_groups to be defined
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As we start to test configured Dependency Groups, we no longer need to rely on the Updater handling empty dependency groups by failing over to a "group everything in one PR" placeholder rule.
The service is now responsible for injecting this in places where it is required, otherwise we should treat an empty
job.dependency_groupsas a sign the feature is not being invoked since this is the target production behaviour.Removing it now will avoid any debugging confusion as we start to test more widely.
Other Changes
As part of making this change I migrated the test that verifies that the
GroupUpdateAllVersionsclass handles mixes of grouped and ungrouped dependencies properly from the "legacy"updater_spec.rbinto the class-specific test.This lead on to a further change in
DependabotSnapshot#ungrouped_dependenciesas it was surprising to find this set empty where no groups were defined.I've added a guard clause to return the full list of
allowed_dependencieswhenDependabotSnapshot#groupsare empty so it can be asserted against in tests consistently andUpdateAllVersionsno longer needs to be aware of whether groups exist or not.