Skip to content
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

Resource selector for ConfigurationPolicy #129

Conversation

dhaiducek
Copy link
Member

@openshift-ci openshift-ci bot requested review from mprahl and qiujian16 October 24, 2024 20:50
@mprahl
Copy link
Member

mprahl commented Oct 25, 2024


### Goals

- Add a `resourceSelector` to `ConfigurationPolicy` to allow the controller to handle objects
Copy link
Member

Choose a reason for hiding this comment

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

Do you think another goal is to consolidate the compliance message for an object-template with a resource selector? One of the main drawbacks with object-templates-raw is you get a separate compliance message per object-template. The use of a namespaceSelector currently consolidates the compliance message but that is limited as you mention in the introduction.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point--the message could probably be refactored to list out the objects per namespace rather than have a message for every object--is that what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Either that or consolidate to something like Pods found as specified: namespace1/name1, namespace2/name2; Pods updated: namespace1/name3 per object-template with a resource selector.

@dhaiducek dhaiducek force-pushed the 128-resource-selector branch from 4a99b26 to 5aa3e94 Compare October 25, 2024 19:17
Comment on lines +36 to +37
- Consolidate compliance messages to list out objects per namespace (one downside to the alternative
`object-templates-raw` is there's a compliance message for each object)
Copy link
Member

Choose a reason for hiding this comment

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

This brings up the question: is the content in the compliance message part of our API which we should be very careful about changing? I suppose we could only change the compliance messages to a new form if the policy uses a resourceSelector, but that seems potentially confusing to users.

Personally, I don't think we need to preserve the existing messages exactly, as long as similar content is still present by default. We could consider exposing a "legacy" message into the CustomMessage template for use, in case a user has some other tooling built on the current format.

@dhaiducek dhaiducek force-pushed the 128-resource-selector branch from 5aa3e94 to c0ec93f Compare October 29, 2024 17:39
Copy link

@yiraeChristineKim yiraeChristineKim left a comment

Choose a reason for hiding this comment

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

/hold for others

@dhaiducek dhaiducek force-pushed the 128-resource-selector branch from c0ec93f to f1910f3 Compare November 1, 2024 20:23
@openshift-ci openshift-ci bot removed the lgtm label Nov 1, 2024
Copy link
Member

@mprahl mprahl left a comment

Choose a reason for hiding this comment

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

/unhold

Copy link

openshift-ci bot commented Nov 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, mprahl, yiraeChristineKim

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 964321d into open-cluster-management-io:main Nov 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource selector for ConfigurationPolicy
4 participants