Skip to content

ROX-28348: Cleanup results on scan config update#15333

Merged
mtodor merged 3 commits intomasterfrom
mtodor/ROX-28348-clean-results-on-scan-config-update
May 30, 2025
Merged

ROX-28348: Cleanup results on scan config update#15333
mtodor merged 3 commits intomasterfrom
mtodor/ROX-28348-clean-results-on-scan-config-update

Conversation

@mtodor
Copy link
Contributor

@mtodor mtodor commented May 16, 2025 •
edited
Loading

Description

This PR is adding cleanup of scan results when scan config (schedule) is changed and clusters or profiles are removed from that schedule.

Additional changes:

  • protection for changing scan config is added (we are using scan config name as a foreign key in the results table)
  • smaller refactoring of tests is done because several test data creation functions could be replaced with one with additional parameters

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
  • modified existing tests

How I validated my change

  • unit tests
  • manual tests with two clusters (after image is built with this PR)

@openshift-ci
Copy link

openshift-ci bot commented May 16, 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

@rhacs-bot
Copy link
Contributor

rhacs-bot commented May 16, 2025 •
edited
Loading

Images are ready for the commit at 4e0f089.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-801-g4e0f089154.

@codecov
Copy link

codecov bot commented May 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.43243% with 13 lines in your changes missing coverage. Please review.

Project coverage is 49.25%. Comparing base (d67f37f) to head (4e0f089).
Report is 68 commits behind head on master.

Files with missing lines Patch % Lines
...ianceoperator/v2/compliancemanager/manager_impl.go 78.94% 8 Missing and 4 partials ⚠️
...mplianceoperator/v2/compliancemanager/singleton.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #15333      +/-   ##
==========================================
+ Coverage   49.21%   49.25%   +0.03%     
==========================================
  Files        2577     2578       +1     
  Lines      189093   189248     +155     
==========================================
+ Hits        93061    93212     +151     
- Misses      88693    88696       +3     
- Partials     7339     7340       +1
Flag Coverage Δ
go-unit-tests 49.25% <82.43%> (+0.03%) ⬆️

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.

@mtodor mtodor force-pushed the mtodor/ROX-28348-clean-results-on-scan-config-update branch from 9c1cf46 to b5d45a4 Compare May 20, 2025 14:59
@mtodor mtodor requested a review from lvalerom May 20, 2025 15:56
@mtodor mtodor marked this pull request as ready for review May 20, 2025 15:56
sourcery-ai[bot]
sourcery-ai bot reviewed May 20, 2025
View reviewed changes
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @mtodor - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@mtodor mtodor mentioned this pull request May 20, 2025
Merged
6 tasks
lvalerom
lvalerom reviewed May 21, 2025
View reviewed changes
Copy link
Contributor

@lvalerom lvalerom left a comment

Choose a reason for hiding this comment

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

Overall it looks good. I haven't looked at the tests yet because I had a suggestion regarding how to delete the checkResults associated with the profiles.

Also, do you know why the checkResults do not get removed automatically by sensor but the scans seem to be removed? My understanding is that sensor should send remove events for both resources but apparently something is not working in the checkResult pipeline (or maybe it's like that by design). I still think we should delete them in central in case there is a problem with sensor.

@mtodor mtodor force-pushed the mtodor/ROX-28348-clean-results-on-scan-config-update branch from b5d45a4 to dafdc36 Compare May 27, 2025 10:21
@mtodor mtodor requested a review from lvalerom May 27, 2025 10:21
@mtodor mtodor force-pushed the mtodor/ROX-28348-clean-results-on-scan-config-update branch from dafdc36 to 4e0f089 Compare May 27, 2025 16:18
lvalerom
lvalerom approved these changes May 29, 2025
View reviewed changes
Copy link
Contributor

@lvalerom lvalerom left a comment

Choose a reason for hiding this comment

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

LGTM

@mtodor mtodor merged commit 8063f18 into master May 30, 2025
90 checks passed
@mtodor mtodor deleted the mtodor/ROX-28348-clean-results-on-scan-config-update branch May 30, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@lvalerom lvalerom lvalerom approved these changes

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

@mtodor @rhacs-bot @lvalerom