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

ACM-18042: Adopt policy best practices (GRC) #7524

Open
wants to merge 5 commits into
base: 2.13_stage
Choose a base branch
from

Conversation

dhaiducek
Copy link
Contributor

@dhaiducek dhaiducek commented Feb 20, 2025

  • Use fully qualified name for policy
  • Remove ManagedClusterConditionAvailable
    • When dealing with policies, this setting is not recommended since it could introduce churn.
  • Add tolerations to Placement
    • Not having these tolerations could introduce churn if the managed cluster becomes unavailable.

ref: https://issues.redhat.com/browse/ACM-18042

Split from:

@dhaiducek dhaiducek requested a review from jc-berger February 20, 2025 18:05
@dhaiducek dhaiducek changed the title Adopt policy best practices (GRC) ACM-18042: Adopt policy best practices (GRC) Feb 21, 2025
@dhaiducek dhaiducek removed the request for review from jc-berger February 21, 2025 01:41
Copy link
Collaborator

@jc-berger jc-berger left a comment

Choose a reason for hiding this comment

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

/lgtm

@@ -182,6 +182,11 @@ spec:
labelSelector:
matchLabels:
cloud: "IBM"
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting seems to be off here.

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed.

+
----
oc delete policies.policy.open-cluster-management.io <policy-name> -n <policy-namespace>
----
+
After your policy is deleted, it is removed from your target cluster or clusters. Verify that your policy is removed by running the following command: `oc get policies.policy.open-cluster-management.io <policy-name> -n <policy-namespace>`

Copy link
Contributor

@swopebe swopebe Feb 26, 2025

Choose a reason for hiding this comment

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

This had a random (+) in the file, which was overused bc it was built with bullets and steps were merged together. Quick format fix.

@dhaiducek
Copy link
Contributor Author

@swopebe If needed, I can handle the rebase/conflicts once the review edits are complete.

Copy link

openshift-ci bot commented Feb 26, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dhaiducek, jc-berger, swopebe

The full list of commands accepted by this bot can be found 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

dhaiducek and others added 5 commits February 26, 2025 14:42
- Use fully qualified name for policy
- Remove `ManagedClusterConditionAvailable`

When dealing with policies, this setting is not
recommended since it could introduce churn.

- Add `tolerations` to `Placement`

Not having these tolerations could introduce churn
if the managed cluster becomes unavailable.

Signed-off-by: Dale Haiducek <[email protected]>
@dhaiducek dhaiducek force-pushed the update-policy-placement branch from 8b849af to 036985c Compare February 26, 2025 20:44
@openshift-ci openshift-ci bot removed the lgtm label Feb 26, 2025
Copy link

openshift-ci bot commented Feb 26, 2025

New changes are detected. LGTM label has been removed.

@dhaiducek
Copy link
Contributor Author

@swopebe I rebased it and think I got it back to where it was previously, but when you have a minute please double check the edits made here to make sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants