-
Notifications
You must be signed in to change notification settings - Fork 19
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
Sync from open-cluster-management-io/config-policy-controller: #189 #691
Conversation
Ref: https://issues.redhat.com/browse/ACM-6652 Signed-off-by: Jason Zhang <jaszhang@redhat.com> (cherry picked from commit c80d51c7bafaf50bb364aeca8b64f61a505350c4)
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
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.
Thanks! Approving, with some comments/questions for the future.
@@ -216,6 +220,7 @@ install-crds: | |||
kubectl apply -f https://raw.githubusercontent.com/stolostron/governance-policy-propagator/main/deploy/crds/policy.open-cluster-management.io_policies.yaml | |||
kubectl apply -f deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml | |||
kubectl apply -f deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml | |||
kubectl apply -f https://raw.githubusercontent.com/operator-framework/api/master/crds/operators.coreos.com_olmconfigs.yaml |
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'm not sure why this is necessary. Isn't this installed with install.sh
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.
It does look like install.sh
handles the CRDs. I'll try it without this.
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 needed a test PR, so I updated it here:
@echo installing olm | ||
curl -L https://github.com/operator-framework/operator-lifecycle-manager/releases/download/v0.26.0/install.sh -o $(LOCAL_BIN)/install.sh | ||
chmod +x $(LOCAL_BIN)/install.sh | ||
$(LOCAL_BIN)/install.sh v0.26.0 |
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.
Future TODO: it's not necessary, but it might be nice if this version were a variable
|
||
return reconcile.Result{}, nil | ||
} | ||
OpLog.Error(err, "Failed to get operator policy") |
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.
Should this log have some context or is that already set elsewhere?
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.
Good note. I want to try using the logger ctrl-runtime now puts into the context (FromContext); I think that would include this information.
deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml
Show resolved
Hide resolved
deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml
Show resolved
Hide resolved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, JustinKuli 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 |
Non-magically mirroring open-cluster-management-io/config-policy-controller#189
Closes #690