Skip to content

[Prototype] Generating grouped update PRs#6663

Closed
brrygrdn wants to merge 17 commits intomainfrom
brrygrdn/prototype-grouping-updates
Closed

[Prototype] Generating grouped update PRs#6663
brrygrdn wants to merge 17 commits intomainfrom
brrygrdn/prototype-grouping-updates

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn commented Feb 14, 2023

⚠️ This is an experimental spike ⚠️

The purpose of this is spike is to create an isolated code line to reimplement Dependabot::Update#run without taking on the need to verify our prototype work as strictly as the production implementation.

It is important to fail-fast on our proposed strategy and test our service's assumptions about PRs without investing more time in refactoring the existing code to make way for a new architecture that we can't be sure will actually work out.

This should not be considered indicative of the final architecture

The Approach

This spike explores grouping in a naive, ecosystem-agnostic way by using the Dependabot::Updater class to drive collecting up the Dependency changes required by a particular group rule - for now we assume a rule of '*', i.e. group everything - and then executing a single FileUpdater and PR creation invocation on the aggregate changes.

For purposes of simplicity, we have disabled concerns around managing the lifecycle of PRs, i.e. superseding and rebasing existing PR, as well as set aside Security-only update strategies since they are already special-cased.

Goals

  1. Flush out general refactors required in the updater/ code to improve quality, test coverage, etc
  2. Determine architectural changes required to Dependabot::Updater to better isolate the UpdateChecker and FileUpdater steps to break the assumption they are always 1:1
  3. Provide a walking skeleton we can attach further experiments within common/lib to in order to test our hypothesis without waiting on (1) and (2) to ship

Non-goals

  1. Write production quality-code
    • This is a highly experimental spike, if we do eventually merge it for continued testing it will remain behind an experiment flag and remain isolated in such a way it cannot impact existing code.
    • This should also be considered a throw-away prototype, so using this in our own scripts is highly discouraged and will not be supported.
  2. Address the dry-run script

@brrygrdn brrygrdn force-pushed the brrygrdn/prototype-grouping-updates branch 2 times, most recently from 6ab5bec to 9d96116 Compare February 14, 2023 14:44
Comment on lines +233 to +237
all_updated_deps = update_batch.values.flatten
logger_info("Updated #{all_updated_deps.count} dependencies")
updated_files = generate_dependency_files_for(all_updated_deps)

create_pull_request(all_updated_deps, updated_files, pr_message(all_updated_deps, updated_files))
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.

This passes existing tests but is too naive to work under most circumstances, this is where we need to replicate the iterative file changes from the dry-run prototype.

@bdragon I think this is essentially where the control plane for the underlying file system would be, with generate_dependency_files_for being executed per row of update_batch.values and the outcome resulting in either a git commit or git clean.

Comment on lines +3 to +11
# FIXME: This file is a copy-paste of `spec/dependabot/updater_spec.rb`
#
# The intent is to run the tests for existing behaviour without the churn
# of refactoring the existing tests into shared examples until we know
# how the code is going to decompose
#
# Tests that don't apply or work properly for grouped updates are skipped
# rather than deleted, and new group-specific tests are added under the
# `run_grouped` block.
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.

As noted, this test is a fork of the existing tests as just acts as a plumb-line for how much coverage we have on the new code, what is currently side-stepped, etc as a way to heatmap where we need to bring up coverage before we start changing the 'real' implementation.

@brrygrdn brrygrdn force-pushed the brrygrdn/prototype-grouping-updates branch from 0d28b4e to 57068cf Compare February 22, 2023 12:10
Copy link
Copy Markdown
Contributor

@landongrindheim landongrindheim left a comment

Choose a reason for hiding this comment

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

Approving to unblock image building workflows 🔓

@brrygrdn brrygrdn force-pushed the brrygrdn/prototype-grouping-updates branch 5 times, most recently from b3f6028 to 5e2e416 Compare February 23, 2023 14:54
@brrygrdn brrygrdn force-pushed the brrygrdn/prototype-grouping-updates branch from d3cfb96 to 6c7b418 Compare February 27, 2023 17:33
@brrygrdn brrygrdn force-pushed the brrygrdn/prototype-grouping-updates branch from 6c7b418 to bc9976c Compare February 27, 2023 17:50
@brrygrdn
Copy link
Copy Markdown
Contributor Author

brrygrdn commented Mar 21, 2023

Closing this out as superseded by #6884

@brrygrdn brrygrdn closed this Mar 21, 2023
@jeffwidman jeffwidman deleted the brrygrdn/prototype-grouping-updates branch June 27, 2024 22:23
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.

2 participants