Skip to content

[Updater] Start splitting the Updater by extracting DependencySnapshot as an input class#6847

Merged
brrygrdn merged 2 commits intomainfrom
brrygrdn/extract-dependency-parsing
Mar 20, 2023
Merged

[Updater] Start splitting the Updater by extracting DependencySnapshot as an input class#6847
brrygrdn merged 2 commits intomainfrom
brrygrdn/extract-dependency-parsing

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn commented Mar 15, 2023

This is another clear-the-way PR for #6663 [Prototype] Generating grouped update PRs.

Depends on #6846

A Dependabot update process has three broad steps:

  1. Fetch the files we need from the repository
  2. Parse the fetched files into a set of dependency information Dependabot can work with
  3. Iterate over the dependency update to make changes to the fetched files which are pushed back to the repo

Currently, step(1) is performed as a single command so we do not need to provide repository credentials to the Update step and it hands files over to step(2) by encoding them into a 'job definition' along with the workload description it used to fetch them.

As we start to break the Dependabot::Updater class out into a set of components we can compose to create grouped updates, one difficulty is that step(2) is performed lazily inside Dependabot::Updater#dependencies which ostensibly functions like a getter method but in reality mixes the responsibility of parsing the files with filtering them according to the update strategy to be performed.

Changes

This PR introduces a new DependencySnapshot object which encapsulates the responsibility of retrieving and decoding the files from step(2) and correctly hydrating them into Dependabot::Dependency objects to result in a value class which describes the starting state of the update.

Rationale

This value class is used like a shim right now, with the parsing still being performed lazily inside Dependabot::Updater#dependencies in order to minimise the blast radius of refactoring the code, so initially it only provides value by reducing the number of arguments Dependabot::Updater requires, but it sets us up for two fast-follows:

  • We can now start splitting out "operation" classes which take on a subset of the overall Dependabot::Updater class responsibilities which accept this object as an argument and proactively evaluate it to start refactoring the call site and flushing out assumptions about performing the parse/filter in a single method.
  • Once we've figured out how to remove some implicit assumptions around the parse/filter method call order, we can push the responsibility of handling parser errors entirely out of the Updater into a discrete precondition which will extract a test surface out of Dependabot::Updater and make it easier to reason about.

Base automatically changed from brrygrdn/extract-updater-error-responsibility to main March 16, 2023 10:37
@brrygrdn brrygrdn marked this pull request as ready for review March 16, 2023 10:38
@brrygrdn brrygrdn requested a review from a team as a code owner March 16, 2023 10:38
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-dependency-parsing branch 2 times, most recently from a688cea to 6083e09 Compare March 17, 2023 11:27
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.

🚀

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.

👌

@brrygrdn
Copy link
Copy Markdown
Contributor Author

I've added one additional commit which uses the new class to push the update_dependency_list private method out the Updater unto the Service.

The Service being responsible for serialising a business object ( DependencySnapshot ) for the ApiClient feels more correct and moves another private method unto an explicit receiver so operation classes can access it without duplicating it. It makes more sense to do that as part of this PR than in a follow up.

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.

Looks good to me! 🚀

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.

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.

@brrygrdn brrygrdn force-pushed the brrygrdn/extract-dependency-parsing branch from 6732de2 to f8f2fa2 Compare March 20, 2023 12:01
@brrygrdn brrygrdn merged commit 8f9e473 into main Mar 20, 2023
@brrygrdn brrygrdn deleted the brrygrdn/extract-dependency-parsing branch March 20, 2023 13:20
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