Relocate reconcilers from wksctl to CAPEI (cluster-api-provider-existinginfra) #286

twelho · 2020-08-05T15:56:04Z

This PR extracts the Cluster and Machine controllers from wksctl out to a separate repo nicknamed CAPEI (cluster-api-provider-existinginfra). The needed controller-related functionality is then vendored in.

Status:

Compiles
Tests pass
Redundant code removed
Code separated on function-level, pieces belonging here moved back
Commits rebased to form a linear removal process (avoid removing and re-adding code)
capi-existinginfra renamed to cluster-api-provider-existinginfra and moved to WW org
wksctl vendors in published CAPEI (go.mod hacks removed)

cc @luxas @bboreham

bboreham · 2020-08-06T15:43:49Z

It strikes me as unlikely we want to move pkg/utilities/path because it relates to the user's home directory, so must be more associated with the CLI command.

Similarly pkg/utilities/manifest because it should only be used when setting up the initial cluster config from files, but it looks like that code is used in many more places than path.

Code like pkg/apis/wksprovider/machine/os/os.go parseCluster() should not be in the pure CAPI provider. It's there for CreateSeedNodeSetupPlan() which is for the bootstrap-from-scratch case. So that function should remain in this repo.
Sounds like os.go needs to be split into two?

Similarly pkg/specs/specs.go has parsing functions. And if they remain in wksctl then package pkg/utilities can also remain.

pkg/qjson is only used in pkg/addons/addon.go, which should be a wksctl function - addons are applied after the cluster comes up. Similarly pkg/registry.

twelho · 2020-08-10T12:15:24Z

Yes most of the code is currently misplaced, I've started by moving pkg/apis/wksprovider/controller/wksctl/machine_controller.go and all of it's recursive file-level dependencies out. I'll add a checkbox for function-level refactoring and moving the right stuff back here.

bboreham

Looking better.

bboreham · 2020-08-13T17:14:03Z

pkg/utilities/version/version_test.go

 	assert.EqualError(t, err, "invalid version \"bad version\": Invalid character(s) found in major number \"bad version\"")
 	assert.False(t, matches)
 }
-
-func TestVersionLessthanWithBothVs(t *testing.T) {


why did we lose these?

version.LessThan (and some other version functions as well) are used by the machine controller, and have thus been moved to capi-existinginfra along with these tests. See capi-existinginfra/pkg/utilities/version.

bboreham · 2020-08-13T17:16:10Z

go.mod

+require github.com/twelho/capi-existinginfra v0.0.0
+
+replace (
+	github.com/twelho/capi-existinginfra => ../capi-existinginfra


this looks like it should have been removed

Will be removed soon when I get the repo published and import paths updated.
Update: Now removed.

Signed-off-by: Dennis Marttinen <dennis@weave.works>

twelho · 2020-08-20T16:00:11Z

All tasks are now completed and tests pass 🎉
Ready for (re)review and merge.

luxas

Amazing work @twelho 💯 🥇 🚀!

Only one small nit 😉

luxas · 2020-08-20T16:11:45Z

.circleci/config.yml

@@ -130,6 +130,8 @@ jobs:
      FOOTLOOSE_CHECKSUM: 0e4e49e81940c5876eafa26607154acd788d9979e9f4a4215f17532a3ea5429a
      KUBECTL_URL: https://dl.k8s.io/v1.18.5/kubernetes-client-linux-amd64.tar.gz
      KUBECTL_CHECKSUM: f6051fa7f715c68d56998d9b4c9be4f08552f3a8427b2c6b9e2e5339dd2929b6
+      CAPEI_URL: https://github.com/weaveworks/cluster-api-provider-existinginfra/archive/v0.0.2.tar.gz
+      CAPEI_CHECKSUM: a2d5d2a0f43a2c0872bcc96265d753868a4f0867f555743a5e280cff73f95cdc


Note for @bboreham @jrryjcksn @palemtnrider: need to update this whenever CAPEI comes with new releases 😄

or, build automation around getting the latest release

luxas · 2020-08-20T16:12:38Z

go.mod

@@ -26,6 +22,7 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.6.1
 	github.com/thanhpk/randstr v1.0.4
+	github.com/weaveworks/cluster-api-provider-existinginfra v0.0.1


bump this to v0.0.2 now?

Yep, updated.

palemtnrider · 2020-08-20T18:34:14Z

Nice work @twelho !

I see this repo still references wksctl-controller and in some places, you had a comment to replace this when capei-controller is available via the build process. Please add an issue so we can track that work.
you used existinginfrav1 when importing cluster-api-provider-existing-infra/apis/cluster.weave.works/v1alpha3. And used capi* when importing other packages when there are conflicts with wksctl packages. For my edification -
- why not capei* instead of capi
- why the difference when importing v1alpha3

This was a huge amount of work thank you for tackling it!

twelho · 2020-08-21T09:34:14Z

Hi @palemtnrider, thanks for your comments!

Issue added for changing wksctl-controller to capei-controller: Replace wksctl-controller with capei-controller when it's published #292
The CAPEI name was not finalized when I did the import renames, but it's a simple change to do now, so I'll make them all consistent (and use the capei prefix). Thanks for noticing!

Signed-off-by: Dennis Marttinen <dennis@weave.works>

twelho · 2020-08-21T11:29:38Z

With the latest push the large diff of pkg/apis/wksprovider/machine/os/os.go is now cleaned up as well. During refactoring I moved functions around, but have now restored them to their original positions in the file.

bboreham

Great work, thanks!

twelho self-assigned this Aug 5, 2020

twelho added chore Related to fix/refinement/improvement of end user or new/existing developer functionality tech-debt Unpleasantness that does (or may in future) affect development labels Aug 5, 2020

twelho force-pushed the vendor-capi-existinginfra branch 3 times, most recently from b8bf241 to 5912743 Compare August 13, 2020 13:12

bboreham reviewed Aug 13, 2020

View reviewed changes

Remove files/functionality moved to cluster-api-provider-existinginfra

36f206e

Signed-off-by: Dennis Marttinen <dennis@weave.works>

twelho force-pushed the vendor-capi-existinginfra branch 10 times, most recently from 3b43c6b to 0a3b8be Compare August 20, 2020 14:19

twelho changed the title ~~[WIP] Relocate reconcilers from wksctl to a dedicated repo~~ Relocate reconcilers from wksctl to CAPEI (cluster-api-provider-existinginfra) Aug 20, 2020

twelho marked this pull request as ready for review August 20, 2020 15:58

luxas approved these changes Aug 20, 2020

View reviewed changes

twelho mentioned this pull request Aug 21, 2020

Replace wksctl-controller with capei-controller when it's published #292

Open

twelho force-pushed the vendor-capi-existinginfra branch from 012e73e to 7c73276 Compare August 21, 2020 09:52

twelho added 2 commits August 21, 2020 13:46

Split os.go, remove unnecessary functions

4510241

Signed-off-by: Dennis Marttinen <dennis@weave.works>

Update import paths to vendored cluster-api-provider-existinginfra

c5b1b92

Signed-off-by: Dennis Marttinen <dennis@weave.works>

twelho added 3 commits August 21, 2020 14:00

Update go.mod, bump dependencies, add linter timeout

aba30d7

Signed-off-by: Dennis Marttinen <dennis@weave.works>

Build CAPEI for integration-tests-container

97b4363

Signed-off-by: Dennis Marttinen <dennis@weave.works>

Bump CAPEI to v0.0.2 in go.mod

60e1ca4

Signed-off-by: Dennis Marttinen <dennis@weave.works>

twelho force-pushed the vendor-capi-existinginfra branch from 7c73276 to 60e1ca4 Compare August 21, 2020 11:01

bboreham approved these changes Aug 21, 2020

View reviewed changes

luxas merged commit 14f846c into weaveworks:master Aug 21, 2020

twelho mentioned this pull request Aug 21, 2020

Refactor towards better CAPI provider structure and improve code quality weaveworks/cluster-api-provider-existinginfra#10

Open

twelho deleted the vendor-capi-existinginfra branch August 21, 2020 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relocate reconcilers from wksctl to CAPEI (cluster-api-provider-existinginfra) #286

Relocate reconcilers from wksctl to CAPEI (cluster-api-provider-existinginfra) #286

twelho commented Aug 5, 2020 •

edited

Loading

bboreham commented Aug 6, 2020 •

edited

Loading

twelho commented Aug 10, 2020

bboreham left a comment

bboreham Aug 13, 2020

twelho Aug 17, 2020

bboreham Aug 13, 2020

twelho Aug 17, 2020 •

edited

Loading

twelho commented Aug 20, 2020

luxas left a comment

luxas Aug 20, 2020

luxas Aug 20, 2020

luxas Aug 20, 2020

twelho Aug 20, 2020

palemtnrider commented Aug 20, 2020

twelho commented Aug 21, 2020

twelho commented Aug 21, 2020

bboreham left a comment

Relocate reconcilers from wksctl to CAPEI (cluster-api-provider-existinginfra) #286

Relocate reconcilers from wksctl to CAPEI (cluster-api-provider-existinginfra) #286

Conversation

twelho commented Aug 5, 2020 • edited Loading

bboreham commented Aug 6, 2020 • edited Loading

twelho commented Aug 10, 2020

bboreham left a comment

Choose a reason for hiding this comment

bboreham Aug 13, 2020

Choose a reason for hiding this comment

twelho Aug 17, 2020

Choose a reason for hiding this comment

bboreham Aug 13, 2020

Choose a reason for hiding this comment

twelho Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

twelho commented Aug 20, 2020

luxas left a comment

Choose a reason for hiding this comment

luxas Aug 20, 2020

Choose a reason for hiding this comment

luxas Aug 20, 2020

Choose a reason for hiding this comment

luxas Aug 20, 2020

Choose a reason for hiding this comment

twelho Aug 20, 2020

Choose a reason for hiding this comment

palemtnrider commented Aug 20, 2020

twelho commented Aug 21, 2020

twelho commented Aug 21, 2020

bboreham left a comment

Choose a reason for hiding this comment

twelho commented Aug 5, 2020 •

edited

Loading

bboreham commented Aug 6, 2020 •

edited

Loading

twelho Aug 17, 2020 •

edited

Loading