Policyset dependencies #58
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, only the spec and annotations of policies were synchronized between root and replicated policies. Labels were not synchronized, and the root and replicated policies could drift apart. This was an inconsistent behavior because when the replicated policy is first created, any labels on the root policy *are* copied over. Signed-off-by: Justin Kulikauskas <[email protected]>
These functions were not as "common" as them being in the "common" pkg would suggest. Now they're closer to where they are used, and future related changes will be able to stay in one place. Signed-off-by: Justin Kulikauskas <[email protected]>
Since whether dependencies are satisfied will be calculated on the managed cluster, and since PolicySets are not propagated to managed clusters, in order to work "as expected" by a user, any PolicySet dependencies should be replaced with the Policies that are in that set. To keep those lists up to date, the root policies need to reconcile whenever the dependent PolicySets are updated, accomplished here with the new dynamic watch library. So now Policies watch items referenced in their templates, as well as relevant PolicySets. Note: when a PolicySet does not exist, the dependency is not changed - it is expected that the status message from the managed cluster (indicating that the set does not exist) will be sufficient for users to understand the issue. Refs: - stolostron/backlog#26181 Signed-off-by: Justin Kulikauskas <[email protected]>
Fun 😆 |
willkutler
reviewed
Oct 26, 2022
| return replicated, nil | ||
| } | ||
|
|
||
| func (r *PolicyReconciler) canonicalizeDependencies( |
There was a problem hiding this comment.
I think a comment describing what this function does would be useful, since the formatted dependency array is not really used in this repo so it might be a bit confusing why we are changing it
willkutler
approved these changes
Oct 26, 2022
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, willkutler 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 looks good to me! Just added a nit about adding some comments to clarify what these dependency formatting functions do. |
ChunxiAlexLuo
added a commit
to ChunxiAlexLuo/governance-policy-propagator
that referenced
this pull request
Oct 28, 2022
Signed-off-by: Chunxi Luo <[email protected]> open-cluster-management-io#58
ChunxiAlexLuo
added a commit
to ChunxiAlexLuo/governance-policy-propagator
that referenced
this pull request
Oct 28, 2022
Signed-off-by: Chunxi Luo <[email protected]> open-cluster-management-io#58
ChunxiAlexLuo
added a commit
to ChunxiAlexLuo/governance-policy-propagator
that referenced
this pull request
Oct 28, 2022
Signed-off-by: Chunxi Luo <[email protected]> open-cluster-management-io#58
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some refactoring to (I think) get things in better shape for the changes, and then this is the main commit message:
Since whether dependencies are satisfied will be calculated on the managed cluster, and since PolicySets are not propagated to managed clusters, in order to work "as expected" by a user, any PolicySet dependencies should be replaced with the Policies that are in that set.
To keep those lists up to date, the root policies need to reconcile whenever the dependent PolicySets are updated, accomplished here with the new dynamic watch library. So now Policies watch items referenced in their templates, as well as relevant PolicySets.
Note: when a PolicySet does not exist, the dependency is not changed - it is expected that the status message from the managed cluster (indicating that the set does not exist) will be sufficient for users to understand the issue.
Refs: