Skip to content

[Updater] Introduce a DependencyChangeBuilder class to complete encapsulation of diff generation#7004

Merged
brrygrdn merged 9 commits intomainfrom
brrygrdn/extract-dependency-change-creation
Apr 6, 2023
Merged

[Updater] Introduce a DependencyChangeBuilder class to complete encapsulation of diff generation#7004
brrygrdn merged 9 commits intomainfrom
brrygrdn/extract-dependency-change-creation

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn commented Apr 5, 2023

We've started the process of isolating the main updater operations into concrete classes by extracting the codelines for:

  • Creating fresh updates for all out of date dependencies
  • Refreshing an existing PR for a single out of date dependency

Next we plan to do the same for Security update workflows, but before this we're focusing on removing duplicate code and extracting business objects to encapsulate some of the concerns. ( #6991, #6989 ).

This pull request completes the work we started a while back where we introduced a DependencyChange object which describes the outcome of an operation in terms of the dependencies that have been updated, the updated dependency files, the base SHA it was generated from, etc.

Until now this was just a value class which was used to simplify the interface with the Service/Client layer, but this Pull Request moves the abstraction further up into the shared logic of actually generating the changed files and filtering the dependencies we attempted to update to just the list that did update.

By consolidating this behaviour in a DependencyChangeBuilder, we can remove a significant amount of duplication.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't we want to still filter out the deps here? updated_deps comes from the update checker, and not from the dependency_change_builder's updated dependency 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.

We still do perform this filtering, it just happens inside the DependencyChangeBuilder now instead

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah never mind, I thought we were duplicating work in the UpdateChecker since both compile_updates_for(dependency, dependency_files) and create_change_for(lead_dependency, updated_dependencies, dependency_files) call out to it.

I think it would make sense to let the DependencyChangeBuilder use the UpdateChecker to fetch updated_dependencies rather than having to pass them in separately in create_change_for

Copy link
Copy Markdown
Contributor Author

@brrygrdn brrygrdn Apr 5, 2023

Choose a reason for hiding this comment

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

Yeah, I've thought a bit about that but a lot of the operation-specific logic differs in terms of how the UpdateChecker is tested before we decide to proceed to actually creating a dependency change.

I think that will end up being the main source of logic that is different for each operation - but there might be a follow-up pass where we can have one class encapsulate most of the UpdateChecker invocation + common checks, then leave space for job-specific additional checks before going on to create the actual change.

Copy link
Copy Markdown
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

This generally looks good to me. I think the smoke tests should be enough to verify that the DependencyChangeBuilder is working as intended

@brrygrdn brrygrdn force-pushed the brrygrdn/extract-dependency-change-creation branch 2 times, most recently from 78b33d6 to 0c03bf2 Compare April 6, 2023 10:50
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-dependency-change-creation branch from 0c03bf2 to a52e2bd Compare April 6, 2023 11:01
@brrygrdn brrygrdn merged commit bc34e7d into main Apr 6, 2023
@brrygrdn brrygrdn deleted the brrygrdn/extract-dependency-change-creation branch April 6, 2023 12:10
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