Skip to content
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

[Grouped Updates] Refactor the DependencyGroupEngine into an object, Improved logging for empty groups #7548

Merged
merged 9 commits into from
Jul 14, 2023

Conversation

brrygrdn
Copy link
Contributor

@brrygrdn brrygrdn commented Jul 11, 2023

Addresses #7075 (comment)

We currently implement the DependencyGroupEngine as a module which uses class variables to act a little like a singleton as we weren't sure how it would be injected into the Updater when we created it so we defaulted to global scope.

With much of the architecture now settled, it makes sense to move away from this global object to instantiate it immediately after we parse the project's manifest files when creating a the DependencySnapshot

This ensures that any fatal issues parsing the rules can be handled here:

begin
dependency_snapshot = Dependabot::DependencySnapshot.create_from_job_definition(
job: job,
job_definition: Environment.job_definition
)
rescue StandardError => e
handle_parser_error(e)
# If dependency file parsing has failed, there's nothing more we can do,
# so let's mark the job as processed and stop.
return service.mark_job_as_processed(Environment.job_definition["base_commit_sha"])
end

Improved handling of empty groups

Right now we quietly swallow any groups which are defined but do not correspond to any dependencies which means we fail over to doing single updates without calling out why the group wasn't attempted.

As part of this work, it became much easier to warn during the job startup when we find an empty group, which results in us logging a message like:

Please check your configuration as there are groups no dependencies match:
- group-a
- group-b

This can happen if:
- the group's 'pattern' rules are misspelled
- your configuration's 'allow' rules do not permit any of the dependencies that match the group
- the dependencies that match the group rules have been removed from your project

At runtime, we will now acknowledge the empty groups exist and specifically log that we are skipping them like so:

Skipping update group for 'group-a' as it does not match any allowed dependencies.

Rebasing

It is possible for us to find when rebasing a grouped PR that the group definition has now become empty due to all dependencies matching the group having been removed.

Rather than the job being a silent no-op, we will now close the pull request with the following comment:

None of your dependencies match this group anymore, you may need to update your configuration file to remove it or change its rules.

TODO

  • Ship a change to the Dependabot service so it is aware of the dependency_group_empty close reason

@brrygrdn brrygrdn requested a review from a team as a code owner July 11, 2023 15:34
@jakecoffman
Copy link
Member

LGTM other than the smoke test failures, makes sense 👍

@brrygrdn
Copy link
Contributor Author

The remaining smoke test failure is actually an issue with the test rig, which I've fixed here: dependabot/smoke-tests#103

This does make me 🤔 about the decision to always parse the group configuration immediately after parsing dependencies in every job as some jobs may not actually involve using the groups like rebases but I think the problem here was ultimately a mandatory array being nil which is a defect we won't see in the real world.

We could push the evaluation of groups later but since it only warns it still seems better to me to perform it at the same call sight as evaluating the manifests and parsing dependencies. In future it might make sense to move the operation split up so we don't bother parsing the files when we are told to update a specific dependency/and or rebase a PR?

@brrygrdn
Copy link
Contributor Author

@jakecoffman This should be good to go now

@brrygrdn brrygrdn force-pushed the brrygrdn/refactor-dependency-group-engine branch from e7804f5 to 15beeeb Compare July 14, 2023 09:13
@brrygrdn brrygrdn merged commit 4e39370 into main Jul 14, 2023
86 checks passed
@brrygrdn brrygrdn deleted the brrygrdn/refactor-dependency-group-engine branch July 14, 2023 11:32
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
…dependency-group-engine

[Grouped Updates] Refactor the DependencyGroupEngine into an object, Improved logging for empty groups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants