-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
[Federation] Fix the comments on FederationNameAnnotation #44472
[Federation] Fix the comments on FederationNameAnnotation #44472
Conversation
Hi @perotinus. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
// applied to all API objects associated with that federation. | ||
// the federation that that a federation control plane component is associated | ||
// with. It must be applied to all the API types that represent that federation | ||
// control plane components in the host cluster. |
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.
@madhusudancs Currently, I've been annotating objects in both the host and joining clusters. Do we just want to annotate objects in the joining cluster?
A minor nit. Reviewed 1 of 1 files at r1. federation/apis/federation/annotations.go, line 20 at r1 (raw file):
s/that that/that/ federation/apis/federation/annotations.go, line 22 at r1 (raw file): Previously, perotinus (Jonathan MacMillan) wrote…
We definitely want to annotate objects in the host cluster. We create a number of objects in the host cluster and we want to keep track of them. Annotating objects in the joining cluster is useful too, esp. if the cluster is part of multiple federations. Feel free to fix the doc string accordingly. Comments from Reviewable |
315b38e
to
3e69ca7
Compare
Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions. federation/apis/federation/annotations.go, line 20 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
D'oh. Fixed. federation/apis/federation/annotations.go, line 22 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Oh, yes, of course! I think that's what I meant to say. Comments from Reviewable |
@k8s-bot ok to test |
/lgtm Reviewed 1 of 1 files at r2. Comments from Reviewable |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: madhusudancs, perotinus
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 44607, 44472, 44482) |
Follow-up for minor issues raised in #42683