-
Notifications
You must be signed in to change notification settings - Fork 97
chore: update sigs.k8s.io/controller-runtime to v0.22.1 #2042
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
Conversation
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
|
Skipping CI for Draft Pull Request. |
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
| client client.Client, | ||
| nonCachedClient client.Client, | ||
| cli client.Client, | ||
| nonCachedCli client.Client, |
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.
Umm, not 100% sure whether I have full context...why was this rename necessary? cli could be interpreted as “command-line interface” in many contexts.
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 will revert
|
|
||
| if done, err := s.syncPermissions(ctx); !done { | ||
| return reconcile.Result{Requeue: true}, false, err | ||
| return reconcile.Result{RequeueAfter: time.Second /**/}, false, err |
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.
typo?
| return reconcile.Result{RequeueAfter: time.Second /**/}, false, err | |
| return reconcile.Result{RequeueAfter: time.Second}, false, err |
pkg/common/chetypes/types.go
Outdated
| DiscoveryClient discovery.DiscoveryInterface | ||
| Scheme *runtime.Scheme | ||
| Client client.Client // TODO remove | ||
| NonCachingClient client.Client // TODO remove |
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.
This comment for the old Client, NonCachingClient fields might get overlooked over time.
Suggest creating a tracking issue before merging this PR.
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 will update the comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia, tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Anatolii Bazko <[email protected]>
|
New changes are detected. LGTM label has been removed. |
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
What does this PR do?
Screenshot/screencast of this PR
N/A
What issues does this PR fix or reference?
N/A
How to test this PR?
OpenShift
on Minikube
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedReviewers
Reviewers, please comment how you tested the PR when approving it.