Skip to content

feat(reporting): Separate partial download and partial email#15356

Merged
lvalerom merged 3 commits intomasterfrom
lvm/separate-partial-download-and-partial-email
May 30, 2025
Merged

feat(reporting): Separate partial download and partial email#15356
lvalerom merged 3 commits intomasterfrom
lvm/separate-partial-download-and-partial-email

Conversation

@lvalerom
Copy link
Contributor

@lvalerom lvalerom commented May 19, 2025 •
edited
Loading

Description

This PR separates the PARTIAL_ERROR state into PARTIAL_SCAN_ERROR_EMAIL and PARTIAL_SCAN_ERROR_DOWNLOAD

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

  • Unit tests updated
  • Manual testing:
    • Deploy in two clusters with Compliance Operator
    • Create a Scan Schedule with both cluster
    • Make one of the scans fail mid scan (e.g. disconnect sensor from central)
    • Query the API and observe the status are no longer PARTIAL_ERROR

@openshift-ci
Copy link

openshift-ci bot commented May 19, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@lvalerom lvalerom changed the base branch from master to lvm/rox-26974-populate-api-fields May 19, 2025 15:41
@rhacs-bot
Copy link
Contributor

rhacs-bot commented May 19, 2025 •
edited
Loading

Images are ready for the commit at 474db4a.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-854-g474db4ad48.

@codecov
Copy link

codecov bot commented May 19, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.64%. Comparing base (7001a81) to head (474db4a).
Report is 16 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #15356      +/-   ##
==========================================
- Coverage   49.25%   48.64%   -0.61%     
==========================================
  Files        2581     2581              
  Lines      189381   189470      +89     
==========================================
- Hits        93275    92174    -1101     
- Misses      88758    90086    +1328     
+ Partials     7348     7210     -138
Flag Coverage Δ
go-unit-tests 48.64% <100.00%> (-0.61%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lvalerom lvalerom mentioned this pull request May 26, 2025
Merged
11 tasks
@lvalerom lvalerom force-pushed the lvm/rox-26974-populate-api-fields branch 2 times, most recently from 6da877f to 4b0c8a3 Compare May 26, 2025 16:43
@lvalerom lvalerom force-pushed the lvm/separate-partial-download-and-partial-email branch from 2eb94bc to 44e7b25 Compare May 27, 2025 13:05
@lvalerom lvalerom force-pushed the lvm/rox-26974-populate-api-fields branch from 3f62977 to d3bdd09 Compare May 27, 2025 16:45
@lvalerom lvalerom force-pushed the lvm/separate-partial-download-and-partial-email branch 2 times, most recently from da1e563 to 3cd067b Compare May 28, 2025 09:45
@lvalerom lvalerom marked this pull request as ready for review May 28, 2025 09:45
@lvalerom lvalerom requested a review from a team as a code owner May 28, 2025 09:45
@lvalerom lvalerom requested review from mtodor and vikin91 May 28, 2025 09:45
@lvalerom lvalerom force-pushed the lvm/rox-26974-populate-api-fields branch from d3bdd09 to 024867e Compare May 28, 2025 10:15
@lvalerom lvalerom force-pushed the lvm/separate-partial-download-and-partial-email branch from 3cd067b to e594d38 Compare May 28, 2025 10:16
@lvalerom lvalerom force-pushed the lvm/rox-26974-populate-api-fields branch from 024867e to dfd2833 Compare May 28, 2025 10:56
@lvalerom lvalerom force-pushed the lvm/separate-partial-download-and-partial-email branch 2 times, most recently from c09d16f to e6683e1 Compare May 28, 2025 10:59
@lvalerom lvalerom force-pushed the lvm/rox-26974-populate-api-fields branch from dfd2833 to 044d0d4 Compare May 28, 2025 14:21
@lvalerom lvalerom force-pushed the lvm/separate-partial-download-and-partial-email branch from e6683e1 to d477e99 Compare May 28, 2025 15:02
Base automatically changed from lvm/rox-26974-populate-api-fields to master May 28, 2025 17:31
@lvalerom lvalerom force-pushed the lvm/separate-partial-download-and-partial-email branch 3 times, most recently from 1c4c448 to 0c3571a Compare May 29, 2025 12:27
@lvalerom lvalerom force-pushed the lvm/separate-partial-download-and-partial-email branch from 0c3571a to cd51e13 Compare May 29, 2025 13:07
@lvalerom
Copy link
Contributor Author

lvalerom commented May 30, 2025

/retest

vikin91
vikin91 approved these changes May 30, 2025
View reviewed changes
Copy link
Contributor

@vikin91 vikin91 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!

I am just curious why we need to distinguish those two? I thought that partial error and notification method are two orthogonal aspects.
Next question: If we have those two new types, when would we use the original ComplianceOperatorReportStatus_PARTIAL_ERROR and when PARTIAL_SCAN_ERROR_DOWNLOAD or PARTIAL_SCAN_ERROR_EMAIL?

@lvalerom
Copy link
Contributor Author

lvalerom commented May 30, 2025

I am just curious why we need to distinguish those two? I thought that partial error and notification method are two orthogonal aspects.

It was a UI request. I'm assuming to make things easier there.

If we have those two new types, when would we use the original ComplianceOperatorReportStatus_PARTIAL_ERROR and when PARTIAL_SCAN_ERROR_DOWNLOAD or PARTIAL_SCAN_ERROR_EMAIL

We do not use PARTIAL_ERROR anymore. We should probably deprecate the field but I didn't want to do it in this PR.

@openshift-ci
Copy link

openshift-ci bot commented May 30, 2025

@lvalerom: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocp-4-18-scanner-v4-install-tests 474db4a link false /test ocp-4-18-scanner-v4-install-tests
ci/prow/ocp-4-12-scanner-v4-install-tests 474db4a link false /test ocp-4-12-scanner-v4-install-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@lvalerom lvalerom merged commit 21cabce into master May 30, 2025
97 of 99 checks passed
@lvalerom lvalerom deleted the lvm/separate-partial-download-and-partial-email branch May 30, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vikin91 vikin91 vikin91 approved these changes

@mtodor mtodor Awaiting requested review from mtodor

Assignees

No one assigned

Labels

area/central

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lvalerom @rhacs-bot @vikin91