Use go modules, update controller-{runtime, tools} to v2 #1054

vincepri · 2019-06-21T00:31:20Z

Signed-off-by: Vince Prignano vincepri@vmware.com

What this PR does / why we need it:

Upgrades controller-runtime to v0.2.0-beta.2.
Upgrades controller-tools to v0.2.0-beta.2.
Upgrades CI fetchtools Kubebuilder to 1.14.1 release.
Removes dep, Gopkg.lock, and Gopkg.toml.
Overhauls Makefile to use new controller-tools utilities.
Adds hack/tools.go file to force-vendor utilities.
Adds hack/ensure-go.sh to check and setup Go for modules.
Removes - from controller names (e.g. machine-controller -> machine_controller).
Updates integration tests to use kubectl@1.14.1 and kind@0.3.0.

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 #1025

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

k8s-ci-robot · 2019-06-21T00:31:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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

akutz

Hi @vincepri,

I need more time to review this, but I did have two initial comments. One is in the code, and the other is this -- is it necessary to tie Go modules to the changes for v2? I see a lot of code changes that appear unrelated to modules.

hack/update-vendor.sh

vincepri · 2019-06-21T04:45:46Z

Current blockers:

CI images have an outdated version of Go (<1.12.1) PR.
Failing tests on Update.Status (details here)

Non blocking issues:

RBAC tags have to be placed at the top of the file, without any code around it to work.
RBAC RoleBinding isn't generated from KubeBuilder/controller-tools anymore.

vincepri · 2019-06-21T15:32:51Z

/test pull-cluster-api-build

vincepri · 2019-06-24T14:31:59Z

/hold

Currently waiting for directions on how to handle the ObjectMeta embedding issue from @DirectXMan12 and others. We can decide to merge this as it is with the fork to temporarily unblock v1alpha2 progress or wait if the timeframe to fix it in controller-tools works.

vincepri · 2019-06-24T19:51:14Z

/hold cancel

vincepri · 2019-06-24T19:51:27Z

/assign @ncdc

ncdc · 2019-06-24T20:08:49Z

I have at least 1 pending review comment that will need fixing before we merge, but I'm still making my way through the diffs. Until I get that submitted:

/hold

hack/pin-dependency.sh

pkg/controller/cluster/cluster_controller.go

Signed-off-by: Vince Prignano <vincepri@vmware.com>

ncdc · 2019-06-24T20:34:55Z

/lgtm

ncdc · 2019-06-24T20:35:06Z

/hold cancel

detiber

Late to the party, but a few things stood out to me when taking a look.

detiber · 2019-06-25T18:20:18Z

cmd/clusterctl/testdata/create-cluster-no-args-invalid-flag.golden

@@ -25,7 +25,7 @@ Global Flags:
      --log-file-max-size uint           Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
      --log-flush-frequency duration     Maximum number of seconds between log flushes (default 5s)
      --logtostderr                      log to standard error instead of files (default true)
-      --master string                    The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.
+      --master --kubeconfig              (Deprecated: switch to --kubeconfig) The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.


This seems like an odd diff here.

I checked with a master build of clusterctl and it seems that's just how the help is printed out

right, this seems broken

detiber · 2019-06-25T18:48:33Z

docs/book/provider_implementations/overview.md

 `kubebuilder` to create an example provider. For more information on `kubebuilder`
 and CRDs in general we highly recommend reading the [Kubebuilder Book][kubebuilder-book].
 Much of the information here was adapted directly from it. The minimal version of
-`kubebuilder` required is [`v1.0.5`][kubebuilder-1.0.5].
+`kubebuilder` required is [`v2.0.0`][kubebuilder-2.0.0].


I suspect there will be more changes needed to the book to accommodate this change, should have an issue to track that work.

Opened #1075

detiber · 2019-06-25T18:53:39Z

pkg/client/clientset_generated/clientset/clientset.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-// Code generated by client-gen. DO NOT EDIT.
+// Code generated by main. DO NOT EDIT.


This is a big ugly, is this because it is run using go run now instead of compiling and running the binary?

detiber · 2019-06-25T18:57:28Z

pkg/controller/controller.go

+// +kubebuilder:rbac:groups=cluster.k8s.io,resources=clusters,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=cluster.k8s.io,resources=machines;machines/status,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=cluster.k8s.io,resources=machinedeployments;machinedeployments/status,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=cluster.k8s.io,resources=machinesets;machinesets/status,verbs=get;list;watch;create;update;patch;delete


Are the annotations no longer supported closer to where there are required?

They don't work if defined elsewhere, that's why I moved them in here

they should work anywhere in the packages that controller-gen is told to read, as long at they're not in, say, the Godoc for a function or something.

detiber · 2019-06-25T19:04:43Z

cmd/clusterctl/validation/validate_cluster_api_objects.go

@@ -43,7 +43,7 @@ func ValidateClusterAPIObjects(ctx context.Context, w io.Writer, c client.Client
 	}

 	machines := &clusterv1alpha1.MachineList{}
-	if err := c.List(ctx, client.InNamespace(namespace), machines); err != nil {
+	if err := c.List(ctx, machines, client.InNamespace(namespace)); err != nil {


What caused the ordering to change here? Is it just related to the updated client generation?

It's from a controller-runtime change to the method signature

kubernetes-sigs/controller-runtime#294

we changed the method signature to be in line with other methods and to support easier option passing.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 21, 2019

k8s-ci-robot requested review from apelisse and caesarxuchao June 21, 2019 00:31

akutz reviewed Jun 21, 2019

View reviewed changes

hack/update-vendor.sh Outdated Show resolved Hide resolved

vincepri force-pushed the controller-runtime-tools-modules branch from b9ddf69 to 08fee7e Compare June 21, 2019 01:18

vincepri mentioned this pull request Jun 21, 2019

Update all cluster-api images to latest kubekins-e2e 1.15 kubernetes/test-infra#13113

Merged

vincepri force-pushed the controller-runtime-tools-modules branch 6 times, most recently from 0a21b7f to 0583f26 Compare June 21, 2019 03:32

gyliu513 mentioned this pull request Jun 21, 2019

Refactor ibm cloud provider use go module kubernetes-sigs/cluster-api-provider-ibmcloud#235

Closed

vincepri force-pushed the controller-runtime-tools-modules branch from 0583f26 to 6f870ec Compare June 21, 2019 15:26

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2019

vincepri force-pushed the controller-runtime-tools-modules branch from 6f870ec to e638a83 Compare June 21, 2019 17:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2019

vincepri force-pushed the controller-runtime-tools-modules branch 6 times, most recently from b20e6cf to 9f7d9c9 Compare June 21, 2019 18:43

vincepri force-pushed the controller-runtime-tools-modules branch 7 times, most recently from 5b63789 to 5270eec Compare June 21, 2019 20:33

vincepri mentioned this pull request Jun 21, 2019

Resize machine deployment fails kubernetes-sigs/cluster-api-provider-aws#787

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019

vincepri force-pushed the controller-runtime-tools-modules branch from 5270eec to eac35da Compare June 24, 2019 19:45

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019

k8s-ci-robot assigned ncdc Jun 24, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019

vincepri force-pushed the controller-runtime-tools-modules branch from eac35da to 2527418 Compare June 24, 2019 20:14

ncdc reviewed Jun 24, 2019

View reviewed changes

hack/pin-dependency.sh Outdated Show resolved Hide resolved

pkg/controller/cluster/cluster_controller.go Show resolved Hide resolved

Use go modules, update controller-{runtime, tools} to v2

cd23cbd

Signed-off-by: Vince Prignano <vincepri@vmware.com>

vincepri force-pushed the controller-runtime-tools-modules branch from 2527418 to cd23cbd Compare June 24, 2019 20:25

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019

k8s-ci-robot merged commit 82c49a9 into kubernetes-sigs:master Jun 24, 2019

detiber reviewed Jun 25, 2019

View reviewed changes

vincepri mentioned this pull request Jun 25, 2019

Umbrella: Kubebuilder v2 support #1075

Closed

gyliu513 mentioned this pull request Sep 20, 2019

Propose migrate from dep to go modules openshift/installer#2384

Closed

shenkonghui mentioned this pull request Mar 1, 2024

invalid value: \"null\": status.creationtimestamp in body must be of type string shenkonghui/issue#197

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use go modules, update controller-{runtime, tools} to v2 #1054

Use go modules, update controller-{runtime, tools} to v2 #1054

vincepri commented Jun 21, 2019 •

edited

Loading

k8s-ci-robot commented Jun 21, 2019

akutz left a comment

vincepri commented Jun 21, 2019 •

edited

Loading

vincepri commented Jun 21, 2019

vincepri commented Jun 24, 2019

vincepri commented Jun 24, 2019

vincepri commented Jun 24, 2019

ncdc commented Jun 24, 2019

ncdc commented Jun 24, 2019

ncdc commented Jun 24, 2019

detiber left a comment

detiber Jun 25, 2019

vincepri Jun 25, 2019

detiber Jun 25, 2019

detiber Jun 25, 2019

vincepri Jun 25, 2019

detiber Jun 25, 2019

ncdc Jun 25, 2019

detiber Jun 25, 2019

vincepri Jun 25, 2019

DirectXMan12 Jun 28, 2019

detiber Jun 25, 2019

ncdc Jun 25, 2019

ncdc Jun 25, 2019

DirectXMan12 Jun 28, 2019

Use go modules, update controller-{runtime, tools} to v2 #1054

Use go modules, update controller-{runtime, tools} to v2 #1054

Conversation

vincepri commented Jun 21, 2019 • edited Loading

k8s-ci-robot commented Jun 21, 2019

akutz left a comment

Choose a reason for hiding this comment

vincepri commented Jun 21, 2019 • edited Loading

vincepri commented Jun 21, 2019

vincepri commented Jun 24, 2019

vincepri commented Jun 24, 2019

vincepri commented Jun 24, 2019

ncdc commented Jun 24, 2019

ncdc commented Jun 24, 2019

ncdc commented Jun 24, 2019

detiber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jun 21, 2019 •

edited

Loading

vincepri commented Jun 21, 2019 •

edited

Loading