Skip to content

[Updater] Extract UpdateVulnerableVersion as an Operation class#6961

Merged
brrygrdn merged 9 commits intomainfrom
brrygrdn/extract-fresh-security-update-operation
Apr 18, 2023
Merged

[Updater] Extract UpdateVulnerableVersion as an Operation class#6961
brrygrdn merged 9 commits intomainfrom
brrygrdn/extract-fresh-security-update-operation

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

Follows on from #6866, #6884, #6939

This branch peels out the Updater code required to accept a job which is generating a new 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

Notes

The one significant deviation from the previous PRs is that this does make test changes. In order to pass the existing tests, some setups need to be completed by ensuring there is a value for job.dependencies.

In real-world conditions, we would never start a Security Update job without at least one, and normally only one, specific target dependency to fix.

This should not realistically result in a change of behaviour, it is essentially removing an unsupported "working by accident" codepath where the Dependency files contain exactly the insecure dependency to be fixed.

@brrygrdn
Copy link
Copy Markdown
Contributor Author

Before merging this, there is some cleanup work I'd like to push forward on the existing three Operation classes to avoid continuing the heavy duplicate since Security Updates have so many more helper methods.

I've created this as a reference to make sure my cleanup aligns with the "next" step in the refactor without breaking the tests.

@brrygrdn brrygrdn force-pushed the brrygrdn/extract-fresh-security-update-operation branch 2 times, most recently from dc905be to a71c449 Compare April 3, 2023 17:52
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-fresh-security-update-operation branch from a71c449 to 4090b0b Compare April 11, 2023 19:22
@brrygrdn brrygrdn marked this pull request as ready for review April 11, 2023 19:22
@brrygrdn brrygrdn requested a review from a team as a code owner April 11, 2023 19:22
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-fresh-security-update-operation branch from b67777f to ab2a8ce Compare April 12, 2023 09:53
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-fresh-security-update-operation branch from 182d198 to 1140fbc Compare April 12, 2023 12:20
@brrygrdn
Copy link
Copy Markdown
Contributor Author

This PR shouldn't merge until the smoke tests have been updated: dependabot/smoke-tests#51

Currently, the security update smoke test goes into the legacy_run codeline by accident.

Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

First time I review these refactorings and I love them!

end
### END: Security Update Helpers

def requirements_to_unlock(checker)
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.

We should be able to fully move this method to UpdateChecker after #5902 and not keep it duplicated for all operations.

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 is great news, that method has been a thorn in our side on this work!

@brrygrdn brrygrdn force-pushed the brrygrdn/extract-fresh-security-update-operation branch from 1140fbc to 93b310e Compare April 18, 2023 12:30
@brrygrdn brrygrdn merged commit 797c9fd into main Apr 18, 2023
@brrygrdn brrygrdn deleted the brrygrdn/extract-fresh-security-update-operation branch April 18, 2023 13:42
@jakecoffman jakecoffman mentioned this pull request Oct 10, 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