Skip to content

[Updater] Incorporate our first pass at grouped updates as an experiment we can ship#6884

Merged
brrygrdn merged 6 commits intomainfrom
brrygrdn/implement-group-update-all-versions-op
Mar 23, 2023
Merged

[Updater] Incorporate our first pass at grouped updates as an experiment we can ship#6884
brrygrdn merged 6 commits intomainfrom
brrygrdn/implement-group-update-all-versions-op

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn commented Mar 21, 2023

This branch supersedes our original spike in #6663

Having established and shipped the "operation" pattern as a way to separate out the code paths for specific types of Dependabot::Job objects to avoid a lot of low-level branching, we can now package our experimental spike into a code structure we can safely ship without risking an impact on production jobs.

This PR introduces the Dependabot:: Updater::Operations::GroupUpdateAllVersions class which applies to all fresh, Version Updates which have a Dependabot::Experiment flag enabled.

This code is currently very lightly tested to confirm that it behaves identically to the existing adapter for a simple unit test case. I plan to add some smoke tests to demonstrate the correct grouping behaviour as a guard against regressions on this basic walking skeleton, but there is a job to be done in fleshing out the tests properly as a follow up.

@brrygrdn brrygrdn requested a review from a team as a code owner March 21, 2023 18:53
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any concerns with casing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok at this point as we are using Dependabot::Dependency objects and the name should be normalised, plus this is currently replicating existing behaviour that hasn't been DRY'd out yet.

Case checking is something we have a few inconsistencies on though, so it's a good spot.

# Return dependencies in a random order, with top-level dependencies
# considered first so that dependency runs which time out don't always hit
# the same dependencies
allowed_deps = allowed_deps.shuffle unless ENV["UPDATER_DETERMINISTIC"]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably remove shuffling within groups and instead shuffle the groups when we have more than one, but for now I think shuffling has the benefit that it might flush out issues compiling changes together if we run them in a random order - if we made this deterministic now it could result in false confidence.

Copy link
Copy Markdown
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brrygrdn brrygrdn force-pushed the brrygrdn/implement-group-update-all-versions-op branch from 19e6eb5 to a60a3cb Compare March 23, 2023 13:58
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.

3 participants