-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-6710: remediations: Normalize remediations' annotations to avoid needlesly marking remediations as Outdated #206
OCPBUGS-6710: remediations: Normalize remediations' annotations to avoid needlesly marking remediations as Outdated #206
Conversation
@jhrozek: This pull request references Jira Issue OCPBUGS-6710, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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/test-infra repository. |
/jira refresh |
@jhrozek: This pull request references Jira Issue OCPBUGS-6710, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
In response to this:
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/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this issue!
So it was the annotation that is causing the remediation to become outdated?
/lgtm
Yes, as far as I could tell, the remediation we were about to apply (the new version) had |
/hold |
Verification pass with 4.13.0-0.nightly-2023-01-31-174014+ code in the PR:
$ oc apply -f -<<EOF
$ oc compliance bind -N test -S test tailoredprofile/testprofile $ oc get suite -w
##Manually apply all the remediations, $ oc get ccr |
just rebased on recent master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
nothing needed from support this is a simple bugfix |
Thanks @jhrozek . "On re-running scans, remediations might have been marked as Outdated I am a little confused. "Remediations might have been marked as Outdated" - so remediations start as Outdated, you would run the scan again, and it would still say Outdated because it did not account for differences in metadata? Did it actually run the scan at all? |
…oid needlesly marking remediations as Outdated Because the remediation contents we Get() from the cluster after they've been applied contain an empty Annotations map, but the remediations we parse from content do not, the comparison always yielded false and the remediatins have been set to Outdated as a result. This patch adds an empty annotations map to remediations before comparing them, so that we compare their normalized version.
Yes, I see how the description was confusing. I reworded it a little, but feel free to suggest even better wording. What happens step-by-step is:
Thank you for the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment on a potential assertion in the test, but we could follow that up.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhrozek, rhmdnd, Vincent056 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Removing the hold since the patch was verified by QE. |
/test e2e-aws Failed due to timeout |
@jhrozek: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-6710 has been moved to the MODIFIED state. In response to this:
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/test-infra repository. |
Because the remediation contents we Get() from the cluster after they've
been applied contain an empty Annotations map, but the remediations we
parse from content do not, the comparison always yielded false and the
remediatins have been set to Outdated as a result.
This patch adds an empty annotations map to remediations before
comparing them, so that we compare their normalized version.
note: I have not ran the e2e test addition yet. I did test the PR manually, though.