-
Notifications
You must be signed in to change notification settings - Fork 27
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
CMP-2130: Implement support for profile versioning #425
Conversation
@rhmdnd: This pull request references CMP-2130 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. 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. |
Need an end-to-end integration test, which shouldn't be too difficult now that the some of the profile versions have landed in the cac/content repository. |
/hold |
Verification pass with 4.14.0-0.nightly-2023-10-16-104540 + CO built with code in the PR: |
/label qe-approved |
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.
Looks good to me.
@Vincent056 Can you check it as well?
/test e2e-aws-parallel Profile parsing timed out, but I wasn't able to recreate it locally. Retesting. |
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
/retest |
This commit adds support for an optional version attribute for Profile custom resources. This attribute is parsed out of the datastream and set on the Profile by the compliance operator. It's not intended for end users to supply their own version.
Verification pass with 4.14.0-rc.7 + code in the PR. |
NERC-CIP profile controls are versioned, and we haven't built that into this construct (we're not sure if we will). From what I can tell the E8 profile uses an "updated date" in place of a version. In these cases, we aren't required to version all profiles before landing the implementation since the version attribute is optional. |
} | ||
|
||
// +kubebuilder:object:root=true | ||
|
||
// Profile is the Schema for the profiles API | ||
// +kubebuilder:resource:path=profiles,scope=Namespaced,shortName=profs;prof | ||
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" |
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.
wondering if we update the profile, do we still have the old creationTimestamp
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.
I think it would, since this patch isn't changing the behavior of the creation time.
Do you think that behavior needs to be different.
In my thinking, the profiles are still created from the profile parsers. Multiple versions of the same profile will have the same creation timestamp since they're all created when the profile parser runs. We don't support a way to update a profile besides uploading a new content image, which creates a new profile, right?
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.
From today's discussion during sync up meeting, I think adding age here makes sense, because we had shown age in the past, and adding this here preserve such behavior.
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 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 |
This PR will go hand-in-hand with ComplianceAsCode/content#11241 |
/retest |
This commit adds support for an optional version attribute for Profile
custom resources. This attribute is parsed out of the datastream and set
on the Profile by the compliance operator. It's not intended for end
users to supply their own version.
Future patches may expand on this concept to support multiple versions
of a single profile.