Skip to content

[Updater] Extract RefreshSecurityUpdatePullRequest into an Operation class#7128

Merged
brrygrdn merged 6 commits intomainfrom
brrygrdn/extract-refresh-security-update-operation
Apr 24, 2023
Merged

[Updater] Extract RefreshSecurityUpdatePullRequest into an Operation class#7128
brrygrdn merged 6 commits intomainfrom
brrygrdn/extract-refresh-security-update-operation

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

Follows on from #6866, #6884, #6939, #6961

This branch is the last of four class extractions to peel out the distinct scenarios the Updater actually handles today in clear terms.

Changes

This branch peels out the Updater code required to accept a job which is refreshing a Dependabot PR for a security update using the typical approach of:

  • Extract the entire code path required from the Updater
  • Remove any code related to non-concerns for this operation type, in this case version updates and rebases
  • Wire it into the Updater to be used for a specific job type, verifying current testing passes

The one deviation is that in the initial commit, I've extracted all Security-related helpers into a mixin to encapsulate and avoid duplicating them since they are identical between the "Create" and "Refresh" scenarios. This also affords removing them from the legacy codeline and adding the mixin instead.

For reference, this class is basically a remix of those introduced in #6939 and #6939

What's next

With this PR, every job we run should now be going through one of the four updater classes instead of the Updater#legacy_run method.

I intend to create a PR that branches from this to delete that code and mop up any test cases that are falling into implicit cases we don't actually run in the real world, but I plan to ship this to verify that no production jobs are leaking into the legacy code first as that could change the remaining scope as it might mean our analysis has some gaps.

@brrygrdn brrygrdn requested a review from a team as a code owner April 20, 2023 15:19
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-refresh-security-update-operation branch from 8736ceb to 7fc19f2 Compare April 20, 2023 15:27
@brrygrdn brrygrdn changed the title Brrygrdn/extract refresh security update operation [Updater] Extract RefreshSecurityUpdatePullRequest into an Operation class Apr 20, 2023
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-refresh-security-update-operation branch from 7fc19f2 to d5fe1ee Compare April 21, 2023 11:42
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/extract-refresh-security-update-operation branch from d5fe1ee to cd69bf4 Compare April 24, 2023 12:22
@brrygrdn brrygrdn merged commit aee7dd2 into main Apr 24, 2023
@brrygrdn brrygrdn deleted the brrygrdn/extract-refresh-security-update-operation branch April 24, 2023 15:17
@pavera pavera mentioned this pull request Apr 24, 2023
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