refactor: patch with context #732
Conversation
Signed-off-by: Miaha Cybersec <[email protected]>
Signed-off-by: Miaha Cybersec <[email protected]>
Signed-off-by: Miaha Cybersec <[email protected]>
Signed-off-by: Miaha Cybersec <[email protected]>
Signed-off-by: Miaha Cybersec <[email protected]>
Signed-off-by: Miaha Cybersec <[email protected]>
Signed-off-by: Miaha Cybersec <[email protected]>
MiahaCybersec
requested review from
jeremyrickard,
sozercan and
ashnamehrotra
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
==========================================
+ Coverage 46.98% 48.79% +1.80%
==========================================
Files 18 18
Lines 2205 2232 +27
==========================================
+ Hits 1036 1089 +53
+ Misses 1111 1083 -28
- Partials 58 60 +2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Miaha Cybersec <[email protected]>
There was a problem hiding this comment.
@MiahaCybersec I have submitted some review comments on the tests and code.
This is good work that will make it easier for the Copa project to on-board new contributors, well done!
My requested changes are largely around improving the story telling around how copa works which will benefit both early adopting end-users and new contributors.
I have one question on the TrivyOpts and why that include non-trivy related fields (there's a bk client in there)
The most important thing to do with work like this is to test it extensively and ensure that no behaviour has changed as a result of the submitted changes.
…tion Extracted BuildKit client configurations from TrivyOpts into a new BkClient struct. Updated related functions and tests to accommodate this change, improving modularity and readability. Signed-off-by: Miaha Cybersec <[email protected]>
Renamed the `TrivyOpts` struct to `ScannerOpts` to better reflect its purpose. Added an explicit channel parameter `ch` for error handling throughout the functions to improve code clarity and maintainability. Added new test cases to cover scenarios with digest-referenced Docker images. Signed-off-by: Miaha Cybersec <[email protected]>
Signed-off-by: Miaha <[email protected]>
|
LGTM, will wait to merge. @MiahaCybersec @RobertKielty and I discussed adding canary testing before merging large changes. |
|
Changing PR to draft until it gets further review 👍 |
|
Any updates on internal testing for this refactoring PR? |
|
@MiahaCybersec we won't be prioritizing a specific internal testing pipeline for this right now. I think if we want to rebase this with new changes for now, the integration tests should be good to test this. |
Closes #723