-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add unique profile ID #502
Add unique profile ID #502
Conversation
/hold for test |
/retest-required |
Verification pass for 4.16.0-0.nightly-2024-04-15-184947 + code in #502:
|
/unhold |
/label qe-approved |
164e6a2
to
6650303
Compare
/retest |
1 similar comment
/retest |
This commit implements unique profile ID feature, we are adding a unique profile ID to Profile, ComplianceScan, and ComplianceCheckResult CRD. The profile UUID is generated from sha1 of <bundlename>-<existing-xccdf-profile-id>
Co-authored-by: Watson Yuuma Sato <wsato@redhat.com>
Add logic to handle tailoredprofile unique profile id, let's generated a unique uuid using tailoredprofile id.
Co-authored-by: Watson Yuuma Sato <wsato@redhat.com>
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.
Nitpick: Should it be ProfileUniqueID
or UniqueProfileID
?
/retest |
Co-authored-by: Watson Yuuma Sato <wsato@redhat.com>
could you help to check again, it looks like I am getting the correct result here |
@Vincent056 I have also observed that profile guild from the scan labels is not the same with the guid.
With below ssb, the label for ocp4-moderate scan should be "compliance.openshift.io/guid=d625badc-92a1-5438-afd7-19526c26b03c", instead of "compliance.openshift.io/guid=eceb9af0-17d4-5c59-9b17-07cfd22a3ba1"
|
Added check to make sure the scan we are going to label for the profile GID come from the actully profile being used for the scan, because two profile can potentially have the same name, we want to take bundle name into consideration
@BhargaviGudi thanks for the review, could you retest with the latest patch thanks:
|
tests/e2e/serial/main_test.go
Outdated
const profileGUIDRHCOSModerate = "eceb9af0-17d4-5c59-9b17-07cfd22a3ba1" | ||
const profileGUIDOCPCIS = "a230315d-3e4a-5b58-b00f-f96f1553e036" | ||
|
||
f.AssertProfileGUIDMatches("ocp4-moderate", f.OperatorNamespace, profileGUIDOCPModerate) |
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.
If this assertion fails, I don't think it will fail the test.
Most of our assertions return an error (using https://pkg.go.dev/fmt#Errorf). But, I think we need to check for that error and handle the failure.
If we were passing in the test suite, we could have the assertion call t.Fatalf()
, but based on the current implementation we're relying on the test to enforce the outcome of the assertion (which I think we could argue that the assertion should handle failing the test, but that would be a broader change we would want to do consistently I think).
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.
That's a good catch!
tests/e2e/serial/main_test.go
Outdated
f.AssertScanGUIDMatches("ocp4-cis-node-master", f.OperatorNamespace, profileGUIDOCPCIS) | ||
f.AssertScanGUIDMatches("ocp4-cis-node-worker", f.OperatorNamespace, profileGUIDOCPCIS) | ||
// check if the profileGUID is correct in the tailored profile's label | ||
f.AssertScanGUIDMatches(tpName, f.OperatorNamespace, profileGUIDTP) |
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.
Similar comment here as to what's above.
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.
This is looking good. I have a couple suggestions for the test, and one ergonomic aspect of reusing the same label across resources.
Verification pass with 4.16.0-0.nightly-2024-05-07-025557 + operator built from #502:
|
/unhold |
/label qe-approved |
3d6c18c
to
0f66275
Compare
In order to be able to search a profile easily, let's move the guid as an label in the profile.
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhmdnd, Vincent056, yuumasato 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 |
9e8ea4d
into
ComplianceAsCode:master
This commit implements unique profile ID feature, we are adding a unique profile ID to Profile, ComplianceScan, and ComplianceCheckResult CRD. The profile UUID is generated from sha1 of
<product-id>-<existing-xccdf-profile-id>
ex.
ocp4-moderate
profile hasredhat_openshift_container_platform_4.1
product id andxccdf_org.ssgproject.content_profile_moderate
profile idQA:
What is the reason for this commit?
What is the business value?
What would be the impact on the user once this goes live?
Would this apply to only new deployments or all the deployments once upgraded?
Would each user have different profile ID? Why is it good/necessary?
https://issues.redhat.com/browse/CMP-2452