✨ Ship CABPK as part of CAPI #1545

vincepri · 2019-10-15T16:32:42Z

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

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

k8s-ci-robot · 2019-10-15T16:32:49Z

[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

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

vincepri · 2019-10-15T16:54:39Z

/test pull-cluster-api-integration

vincepri · 2019-10-15T16:56:39Z

/test pull-cluster-api-integration

vincepri · 2019-10-15T16:56:44Z

/test pull-cluster-api-make

vincepri · 2019-10-15T16:56:54Z

Seems there are some transient issues with docker images

vincepri · 2019-10-15T16:57:32Z

Makefile

@@ -286,7 +292,6 @@ clean-release: ## Remove the release folder
 verify:
 	./hack/verify-boilerplate.sh
 	./hack/verify-doctoc.sh
-	./hack/verify-generated-files.sh


This is repeated in verify-gen target

vincepri · 2019-10-15T16:58:16Z

/test pull-cluster-api-integration
/test pull-cluster-api-make

vincepri · 2019-10-15T16:58:29Z

/assign @chuckha @ncdc

vincepri · 2019-10-15T18:27:05Z

/test pull-cluster-api-integration
/test pull-cluster-api-make

detiber

Some comments...

How would one add a validating webhook for cabpk with this change?
With this change we lose the "reference implementation" for creating a bootstrap provider, do we need to create an issue to replace that (or provide docs)?

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

bootstrap/kubeadm/.golangci.yml

bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go

detiber · 2019-10-15T20:36:42Z

bootstrap/kubeadm/Makefile

@@ -1,239 +0,0 @@
-# Copyright 2019 The Kubernetes Authors.


Are we losing any targets or validations that would be helpful to the larger project?

The Makefile is still available in the old repository, we can port as needed. I didn't miss any targets for basic interactions

I'd prefer not defaulting to untracked work to be done at a later time, can you create an issue with the actual differences that exist to resolve?

There is really nothing that's particular to CABPK in the repository today https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/blob/master/Makefile

Just a quick comparison uncovers these differences to me:

make test generates a coverage profile in CABPK, but not CAPI

the lint and lint-full targets different golang-ci configs (but that can be tracked via an issue specific for golang-ci config)

CABPK has a run target for just running the manager binary

CABPK also has install and deploy targets that are not present in CAPI

If we are removing previous functionality from dev workflow, then we should have buy in for this from the cluster-api-bootstrap-provider-kubeadm-maintainers group, since they have delegated ownership of this part of the project.

bootstrap/kubeadm/kubeadm/v1beta1/utils.go

main.go

detiber · 2019-10-15T20:41:37Z

bootstrap/kubeadm/hack/verify-shellcheck.sh

@@ -1,54 +0,0 @@
-#!/usr/bin/env bash


I don't think we are doing any shellcheck validation in the top level verify targets/scripts, do we want to lose that?

I'm not sure if there are other verification scripts we are losing here additionally.

We can port later, can you open an issue?

Could you create the issue(s)? I'm not sure the full scope of removals from just a PR review and would rather not have to try diff CABPK/CAPI to determine the delta.

detiber · 2019-10-15T20:44:34Z

As a side effect of this, we'll need to create a migration plan for v1alpha2 -> v1alpha3.

vincepri · 2019-10-15T21:29:15Z

How would one add a validating webhook for cabpk with this change?

That's a good question, @liztio has started working on the related issue

With this change we lose the "reference implementation" for creating a bootstrap provider, do we need to create an issue to replace that (or provide docs)?

I don't think we lose much, apart from the kubebuilder boilerplate, the important bits (types and controllers) are still there.

As a side effect of this, we'll need to create a migration plan for v1alpha2 -> v1alpha3.

Opened #1550

detiber · 2019-10-16T13:54:45Z

With this change we lose the "reference implementation" for creating a bootstrap provider, do we need to create an issue to replace that (or provide docs)?

I don't think we lose much, apart from the kubebuilder boilerplate, the important bits (types and controllers) are still there.

I think we lose much more than that...

We lose the testing and automation rigging we are doing around the image building and publishing.
We lose the manager bits that expose what could be required flags for functionality.
We now make getting to a working example of an alternate implementation:
- create kubebuilder scaffolding for project
- copy types/controllers from bootstrap/kubeadm subdirectory (or just use as a guide)
- make modifications to the Makefile for alignment with other cluster-api projects
- potentially reworking go.mod for alignment with cluster-api project
- having to reverse engineer manager difference and requirements from a subset of the capi manager.

I'm not bringing these up to block the change, we just need a story around how developers can create their own bootstrap provider now (example, scaffolding, docs, whatever), since the guidance until now for v1alpha2 was to look at CABPK as the reference implementation.

ncdc · 2019-10-16T15:00:13Z

We lose the testing and automation rigging we are doing around the image building and publishing.

The CABPK image?

We lose the manager bits that expose what could be required flags for functionality.

Currently this is --bootstrap-token-ttl. That needs to be addressed in this PR.

We now make getting to a working example of an alternate implementation [a lot harder]

Would a walkthrough in documentation be a sufficient replacement?

detiber · 2019-10-16T15:05:33Z

We lose the testing and automation rigging we are doing around the image building and publishing.

The CABPK image?

The process for getting to a working bootstrap provider in general, not necessarily targeted at CABPK specifically.

We lose the manager bits that expose what could be required flags for functionality.

Currently this is --bootstrap-token-ttl. That needs to be addressed in this PR.

Yes, but in this case it's not a requirement that it be exposed from other bootstrap providers. For example if we wanted to mandate that concurrency or metrics be exposed as part of the requirements for a bootstrap provider it isn't clear which may be requirements for a bootstrap provider vs core components.

We now make getting to a working example of an alternate implementation [a lot harder]

Would a walkthrough in documentation be a sufficient replacement?

Definitely. we just need to have some plan for replacement (or a properly prioritized and tracked issue to determine a replacement for the release).

ncdc · 2019-10-16T15:08:24Z

Yes, but in this case it's not a requirement that it be exposed from other bootstrap providers.

I don't think it needs to be?

For example if we wanted to mandate that concurrency or metrics be exposed as part of the requirements for a bootstrap provider it isn't clear which may be requirements for a bootstrap provider vs core components.

I would view this as a documentation task around requirements for implementing a bootstrap provider. I don't think the location of the CABPK code changes this?

detiber · 2019-10-16T15:14:00Z

I would view this as a documentation task around requirements for implementing a bootstrap provider. I don't think the location of the CABPK code changes this?

@ncdc My point was more to enumerate all of the things we potentially lose as CABPK being the reference implementation vs documentation for implementing a bootstrap provider. In this case the example I gave about command line arguments was not applicable. I was trying to point out that there was more being lost than just kubebuilder boilerplate.

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

detiber · 2019-10-16T16:27:31Z

I created #1559, no further blockers from my end.

/lgtm

Update Bazel

031bbd8

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

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

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

k8s-ci-robot requested review from chuckha and detiber October 15, 2019 16:32

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 15, 2019

vincepri added this to the v0.3.0 milestone Oct 15, 2019

vincepri force-pushed the batteries-included branch 3 times, most recently from 7f9dd28 to e50e824 Compare October 15, 2019 16:46

vincepri added 2 commits October 15, 2019 09:48

Update generated files

dd2e52b

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

Update module dependencies

b23a29e

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

vincepri force-pushed the batteries-included branch 2 times, most recently from 1ef1f78 to 14f8c47 Compare October 15, 2019 16:54

vincepri commented Oct 15, 2019

View reviewed changes

k8s-ci-robot assigned chuckha and ncdc Oct 15, 2019

vincepri force-pushed the batteries-included branch 4 times, most recently from 40daae5 to fa60ccf Compare October 15, 2019 17:38

vincepri mentioned this pull request Oct 15, 2019

⚠️ ClusterName required on Machine/MachineSet/MachineDeployment #1539

Merged

detiber reviewed Oct 15, 2019

View reviewed changes

vincepri force-pushed the batteries-included branch from fa60ccf to 756cb96 Compare October 15, 2019 21:26

vincepri force-pushed the batteries-included branch from 756cb96 to 8b0aac9 Compare October 16, 2019 15:06

vincepri force-pushed the batteries-included branch from 8b0aac9 to 4b0af9d Compare October 16, 2019 15:14

✨ Ship CABPK as part of CAPI

94f8cfa

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

vincepri force-pushed the batteries-included branch from 4b0af9d to 94f8cfa Compare October 16, 2019 15:22

vincepri mentioned this pull request Oct 16, 2019

🏃 Enable more golangci-linters #1556

Merged

detiber mentioned this pull request Oct 16, 2019

Add documentation guide for how to implement a bootstrap provider #1559

Closed

k8s-ci-robot assigned detiber Oct 16, 2019

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

k8s-ci-robot merged commit d9e09a7 into kubernetes-sigs:master Oct 16, 2019

vincepri mentioned this pull request Oct 16, 2019

[cabpk] Remove CAPI Crds #1585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Ship CABPK as part of CAPI #1545

✨ Ship CABPK as part of CAPI #1545

vincepri commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

detiber left a comment

detiber Oct 15, 2019

vincepri Oct 15, 2019

detiber Oct 15, 2019

vincepri Oct 15, 2019 •

edited

Loading

detiber Oct 15, 2019

detiber Oct 16, 2019

detiber Oct 15, 2019

vincepri Oct 15, 2019

detiber Oct 15, 2019

vincepri Oct 15, 2019

detiber commented Oct 15, 2019

vincepri commented Oct 15, 2019

detiber commented Oct 16, 2019

ncdc commented Oct 16, 2019

detiber commented Oct 16, 2019

ncdc commented Oct 16, 2019

detiber commented Oct 16, 2019

detiber commented Oct 16, 2019

✨ Ship CABPK as part of CAPI #1545

✨ Ship CABPK as part of CAPI #1545

Conversation

vincepri commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

Choose a reason for hiding this comment

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 2019

vincepri commented Oct 15, 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

vincepri Oct 15, 2019 • edited Loading

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

detiber commented Oct 15, 2019

vincepri commented Oct 15, 2019

detiber commented Oct 16, 2019

ncdc commented Oct 16, 2019

detiber commented Oct 16, 2019

ncdc commented Oct 16, 2019

detiber commented Oct 16, 2019

detiber commented Oct 16, 2019

vincepri Oct 15, 2019 •

edited

Loading