-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Remove redundant GetRESTConfig in KCP Management.GetWorkloadCluster #9448
Conversation
Signed-off-by: Stefan Büringer buringerst@vmware.com
restConfig.CAData = clientConfig.CAData | ||
restConfig.CAFile = clientConfig.CAFile | ||
restConfig.Host = clientConfig.Host | ||
|
||
// Retrieves the etcd CA key Pair |
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 only made sense with the old code. Since we moved to CCT there is no reason to call GetRESTConfig twice.
Old code:
func (m *Management) GetWorkloadCluster(ctx context.Context, clusterKey client.ObjectKey) (WorkloadCluster, error) {
// TODO(chuckha): Inject this dependency.
// TODO(chuckha): memoize this function. The workload client only exists as long as a reconciliation loop.
restConfig, err := remote.RESTConfig(ctx, KubeadmControlPlaneControllerName, m.Client, clusterKey)
if err != nil {
return nil, err
}
restConfig.Timeout = 30 * time.Second
if m.Tracker == nil {
return nil, errors.New("Cannot get WorkloadCluster: No remote Cluster Cache")
}
c, err := m.Tracker.GetClient(ctx, clusterKey)
if err != nil {
return nil, err
}
clientConfig, err := m.Tracker.GetRESTConfig(ctx, clusterKey)
if err != nil {
return nil, err
}
// Make sure we use the same CA and Host as the client.
// Note: This has to be done to be able to communicate directly on self-hosted clusters.
restConfig.CAData = clientConfig.CAData
restConfig.CAFile = clientConfig.CAFile
restConfig.Host = clientConfig.Host
/cc @Levi080513 @jessehu |
@sbueringer: GitHub didn't allow me to request PR reviews from the following users: Levi080513, jessehu. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
@@ -112,26 +112,11 @@ func (m *Management) GetWorkloadCluster(ctx context.Context, clusterKey client.O | |||
restConfig = rest.CopyConfig(restConfig) | |||
restConfig.Timeout = 30 * time.Second | |||
|
|||
if m.Tracker == nil { |
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 think this nil check is pretty useless. If it would be a problem it would panic in l.108 at the moment. And if it wouldn't, our e2e tests would never be green if we always return an error
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.
/lgtm
LGTM label has been added. Git tree hash: 151b53325673ba9d5d5fe3fdd86c0c2bad868e08
|
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.
/area provider/control-plane-kubeadm
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: killianmuldoon 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 |
/cherry-pick release-1.5 |
@killianmuldoon: #9448 failed to apply on top of branch "release-1.5":
In response to this:
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'll try again after the first PR merges. |
/cherry-pick release-1.5 |
@killianmuldoon: #9448 failed to apply on top of branch "release-1.5":
In response to this:
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. |
/cherry-pick release-1.5 |
@killianmuldoon: new pull request created: #9451 In response to this:
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. |
/cherry-pick release-1.4 |
@killianmuldoon: new pull request created: #9453 In response to this:
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. |
Signed-off-by: Stefan Büringer buringerst@vmware.com
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #