⚠️ Change remote.NewClusterClient to return a controller-runtime client.Client #1633

noamran · 2019-10-23T19:37:46Z

What this PR does / why we need it:
Updating the remote package -

The ClusterClient interface has been removed.
remote.NewClusterClient now returns a sigs.k8s.io/controller-runtime/pkg/client Client.

Which issue(s) this PR fixes :
Fixes #1606

k8s-ci-robot · 2019-10-23T19:38:00Z

Hi @noamran. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ncdc

Looking great, thanks! Please run gofmt and goimports 😄

controllers/machine_controller.go

controllers/machine_controller_noderef.go

controllers/machineset_controller.go

controllers/machineset_status.go

controllers/machine_controller.go

ncdc · 2019-10-23T20:44:54Z

/ok-to-test

chuckha

Lovely

/approve

Thanks for including the changes for v1alpha2 to v1alpha3 😄

/assign @vincepri

for lgtm

k8s-ci-robot · 2019-10-25T21:26:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, noamran

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:

~~OWNERS~~ [chuckha]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vincepri

This looks great! Some comments around the use of Scheme, and a note about the logic to use the local cluster client given that the Cluster resource is required in v1alpha3

controllers/machine_controller.go

vincepri · 2019-10-26T15:03:45Z

controllers/machine_controller.go

 func (r *MachineReconciler) deleteNode(ctx context.Context, cluster *clusterv1.Cluster, name string) error {
 	logger := r.Log.WithValues("machine", name)
-
+	var c client.Client


Suggested change

var c client.Client

c := r.Client

and then change the if condition to overwrite this value if the cluster isn't nil, removing the need of an else

controllers/machine_controller.go

controllers/machineset_controller.go

vincepri · 2019-10-26T15:06:30Z

controllers/machineset_status.go

 }

 func (r *MachineSetReconciler) getMachineNode(cluster *clusterv1.Cluster, machine *clusterv1.Machine) (*corev1.Node, error) {
+	var c client.Client


Same suggestion as above to simplify these lines

controllers/machine_controller.go

controllers/remote/cluster.go

controllers/machine_controller.go

controllers/machineset_controller.go

controllers/machineset_status.go

controllers/remote/cluster.go

ncdc · 2019-10-29T16:53:48Z

Could you also please update

cluster-api/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

Line 566 in 7816c87

// TODO remove this when remote.NewClusterClient returns a client.Client

?

controllers/cluster_controller.go

controllers/machinedeployment_controller.go

controllers/remote/cluster.go

controllers/remote/cluster_test.go

controllers/machine_controller.go

ncdc · 2019-10-30T18:27:52Z

FYI @akutz @andrewsykim @rudoi @CecileRobertMichon @juan-lee @michaelgugino, this is a small (but positive!) breaking API change to the controllers/remote package for v0.3.x.

…Client

chuckha

I don't see any more issues to address

/lgtm

chuckha · 2019-11-06T20:12:27Z

/test pull-cluster-api-integration

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 23, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2019

k8s-ci-robot requested review from randomvariable and vincepri October 23, 2019 19:38

ncdc reviewed Oct 23, 2019

View reviewed changes

noamran force-pushed the 1606 branch from d4144cb to 1111573 Compare October 23, 2019 19:49

ncdc reviewed Oct 23, 2019

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

noamran force-pushed the 1606 branch from 1111573 to 602e491 Compare October 23, 2019 19:55

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 23, 2019

ncdc added this to the v0.3.0 milestone Oct 23, 2019

noamran force-pushed the 1606 branch 3 times, most recently from eb544c4 to d506c93 Compare October 25, 2019 00:42

noamran changed the title ~~[WIP] ⚠️ Change remote.NewClusterClient to return a controller-runtime client.Client~~ ⚠️ Change remote.NewClusterClient to return a controller-runtime client.Client Oct 25, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2019

chuckha approved these changes Oct 25, 2019

View reviewed changes

k8s-ci-robot assigned vincepri Oct 25, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2019

vincepri reviewed Oct 26, 2019

View reviewed changes

noamran force-pushed the 1606 branch from d506c93 to 3fd8ee1 Compare October 28, 2019 16:33

ncdc reviewed Oct 28, 2019

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

ncdc reviewed Oct 28, 2019

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

ncdc reviewed Oct 28, 2019

View reviewed changes

noamran force-pushed the 1606 branch from 3591d77 to d01b700 Compare October 29, 2019 05:50

ncdc reviewed Oct 29, 2019

View reviewed changes

controllers/cluster_controller.go Outdated Show resolved Hide resolved

controllers/machinedeployment_controller.go Outdated Show resolved Hide resolved

ncdc reviewed Oct 29, 2019

View reviewed changes

controllers/remote/cluster.go Outdated Show resolved Hide resolved

noamran force-pushed the 1606 branch from d01b700 to 4dedb7a Compare October 29, 2019 18:27

ncdc reviewed Oct 29, 2019

View reviewed changes

controllers/remote/cluster_test.go Outdated Show resolved Hide resolved

noamran force-pushed the 1606 branch from 4dedb7a to fd82ed3 Compare October 29, 2019 21:45

ncdc reviewed Oct 30, 2019

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

noamran force-pushed the 1606 branch 2 times, most recently from fa930d5 to 311492d Compare October 30, 2019 19:50

Change remote.NewClusterClient to return a controller-runtime client.…

c96c5c6

…Client

noamran force-pushed the 1606 branch from 311492d to c96c5c6 Compare November 1, 2019 23:30

chuckha approved these changes Nov 6, 2019

View reviewed changes

k8s-ci-robot assigned chuckha Nov 6, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2019

k8s-ci-robot merged commit 9b14be0 into kubernetes-sigs:master Nov 6, 2019

noamran deleted the 1606 branch January 9, 2020 18:09

noamran mentioned this pull request Jan 22, 2020

REQUEST: New membership for noamran kubernetes/org#1587

Closed

6 tasks

⚠️ Change remote.NewClusterClient to return a controller-runtime client.Client #1633

⚠️ Change remote.NewClusterClient to return a controller-runtime client.Client #1633

Uh oh!

Conversation

noamran commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 23, 2019

Uh oh!

ncdc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncdc commented Oct 23, 2019

Uh oh!

chuckha left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 25, 2019

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincepri Oct 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vincepri Oct 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncdc commented Oct 29, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncdc commented Oct 30, 2019

Uh oh!

chuckha left a comment

Choose a reason for hiding this comment

Uh oh!

chuckha commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

noamran commented Oct 23, 2019 •

edited

Loading