Refactor init code into setup package #1161

ANeumann82 · 2019-12-10T16:55:26Z

What this PR does / why we need it:

Clean up the init code that sets up the cluster, installs prerequisites and CRDs and deploys the manager.

Generic handling for prerequisites that are K8s resources
Preparations for validating the full installation
Generalized generation of YAML for --dry-run --output yaml

alenkacz

I must say I don't understand why we introduced a new package named 'setup'. Could you share the motivation? I am kind of against it - in KUDO world I feel like this process is called init and we should stick to using this name even in the internal package structure so I am kind of leaning toward the manifest code being in the init package.

Also could you help me figure out what are the interesting pieces to review here? It's 700 lines and as I went over it most is just moved code and it's hard to find the pieces that are actually worth reviewing (and I think there's probably just few for now). Could you maybe put comments to them? So far I assume it's the cleaned up handling in the /cmd/init.go. Is there more?

alenkacz · 2019-12-11T08:55:22Z

pkg/kudoctl/cmd/init.go

-		var mans []string
-
-		crd, err := cmdInit.CRDs().AsYaml()
+		manifests, err := setup.AsYamlManifests(opts, initCmd.crdOnly)


nice 👍 much cleaner

ANeumann82 · 2019-12-11T09:18:35Z

I must say I don't understand why we introduced a new package named 'setup'. Could you share the motivation? I am kind of against it - in KUDO world I feel like this process is called init and we should stick to using this name even in the internal package structure so I am kind of leaning toward the manifest code being in the init package.

So, the motivation for a new package in general is that the whole init process will become bigger with migrations, upgrading, etc. I don't think it should be in the cmd (sub)package. It will probably be called from non-cmd places as well, to validate the installation, and I (personally) like the separation between the actual cmd code and the business logic that is executed from the cmds.

As for the name setup instead of init- Never mind. I will change it back to init. I was under the impression that init is an invalid prefix, at some point I always had to prefix the import with an alias.

I'll add some comments in the code and in the review to make it easier to find the details

ANeumann82 · 2019-12-11T09:23:19Z

Ah. There it is. I just renamed setup to init, and now I get:

# github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo
../util/kudo/kudo.go:13:2: cannot import package as init - init must be a func
../util/kudo/kudo.go:51:8: undefined: init

alenkacz · 2019-12-11T09:57:09Z

hmm but we already had package init, so the name cannot be a problem? 🤔 pkg/kudoctl/cmd/init/ - that is also package init, just somewhere else.

So what would you think of having package init with sub-package migrations when migrations come in... What do you think about something like this:

- init
-- wait.go (?) or types.go or whatever we want to have on the top level
-- manifests
--- crds.go
--- manager.go
--- webhook.go
--- etc.
-- migrations

ANeumann82 · 2019-12-11T10:18:31Z

Well, you can have a package init. You just have to alias it every time you import it, aka

import (
	cmdinit "github.com/kudobuilder/kudo/pkg/kudoctl/cmd/init"
)

And at least IntelliJ doesn't do that automatically. (And doesn't complain about init. Only the go compiler does :-/)

About the sub packages: That was what I initially started with. Then it got kinda messy, because there are the options, which are passed into the init-code. As it's used from outside, it would be nice to have it init.Options, or setup.Options, but if it's living in the main package, then the sub-packages can't use it, because of circular dependencies.

I've read a bit about packages in go, and decided to not use sub-packages for the moment, as that seems to be the go-way. tbh, I'd prefer to having them, but that seems not to be that easy.

ANeumann82 · 2019-12-11T10:22:37Z

pkg/kudoctl/setup/prereqs.go

+type k8sResource interface {
+	Install(client *kube.Client) error
+	Validate(client *kube.Client) error
+	AsRuntimeObj() []runtime.Object
+}


This is one of the nicer things of the refactoring: There's an abstraction for one or more manifests as a prerequisite, currently implemented in each of the prereq_*.go files. We can extend that with methods for migration, deletion, etc.

pkg/kudoctl/setup/prereq_serviceaccount.go

ANeumann82 · 2019-12-11T10:27:43Z

pkg/kudoctl/setup/setup.go

@@ -0,0 +1,115 @@
+package setup


This is now the main entry point for initialization of the cluster - At some point this should also provide functionality to validate the installation, upgrade the KUDO installation, etc.

alenkacz · 2019-12-11T10:36:55Z

I've read a bit about packages in go, and decided to not use sub-packages for the moment, as that seems to be the go-way. tbh, I'd prefer to having them, but that seems not to be that easy.

Can you share some background to this - like some articles you read? I am just curious. I have not read much about packages in go in particular but to me this is just a general problem of encapsulation so I want to have packages that encapsulate code that somehow relates and that is changed in a similar situations, so from that point of view since migrations are very init specific for us right now, I would not mind having them as a sub-package.

Fixed integration test that expected the custom SA and RB manifests in yaml output #1165

ANeumann82 · 2019-12-11T12:16:54Z

Can you share some background to this - like some articles you read? I am just curious. I have not read much about packages in go in particular but to me this is just a general problem of encapsulation so I want to have packages that encapsulate code that somehow relates and that is changed in a similar situations, so from that point of view since migrations are very init specific for us right now, I would not mind having them as a sub-package.

This is the article I read:
https://dave.cheney.net/practical-go/presentations/qcon-china.html#_project_structure

Regarding encapsulation: I feel that go isn't really big on encapsulation. It mostly seems to be organized by conventions instead of hard rules or by means of language features. As i'm not sure how exactly we're going to implement the migrations/upgrade process, I think it's fine as it is now, but if we end up with custom migration code, I don't mind having that in a sub package.

zen-dog · 2019-12-11T13:08:39Z

As discussed offline, k8s and KUDO both adopted the types-pattern for dealing with circular dependency hell. setup package would look like:

pkg/setup
├── types.go
├── init
│   ├── init.go
│   └── prereqs.go
├── migrations
│   └── ...

where types.go would have all public types so that it can be imported by all packages (above and sub) and avoid circular dependencies.

ANeumann82 · 2019-12-11T14:17:16Z

I now made a refactoring attempt that works like this:

pkg/kudoctl/kudoinit/
|-- crd
|   `-- crds.go
|-- manager
|   `-- manager.go
|-- prereq
|   |-- prereq_namespace.go
|   |-- prereq_serviceaccount.go
|   |-- prereq_webhook.go
|   `-- prereqs.go
|-- setup
|   `-- setup.go
`-- types.go

which is similar to yours, that might work as well. Not committed and pushed yet, as i'm not totally sold, but it does look a bit more organized.

# Conflicts: # pkg/kudoctl/cmd/init.go # pkg/kudoctl/cmd/init/prereqs.go

zen-dog · 2019-12-11T15:24:57Z

which is similar to yours, that might work as well. Not committed and pushed yet, as i'm not totally sold, but it does look a bit more organized.

I like it!

alenkacz · 2019-12-11T16:46:25Z

which is similar to yours, that might work as well. Not committed and pushed yet, as i'm not totally sold, but it does look a bit more organized.

Nice! Please drop the prereq_ prefixes in go files, otherwise 👍

alenkacz

Loving the new structure ❤️

zen-dog

I left a few comments so PTAL. But overall the new structure is a win!

├── crd
│   └── crds.go
├── manager
│   └── manager.go
├── prereq
│   ├── namespace.go
│   ├── prereqs.go
│   ├── serviceaccount.go
│   └── webhook.go
├── setup
│   ├── setup.go
│   └── wait.go
└── types.go```
❤️

zen-dog · 2019-12-12T10:50:56Z

pkg/kudoctl/cmd/init_integration_test.go

@@ -337,7 +339,7 @@ func TestNoErrorOnReInit(t *testing.T) {
 	assert.IsType(t, &meta.NoKindMatchError{}, testClient.Create(context.TODO(), instance))

 	// Install all of the CRDs.
-	crds := cmdinit.CRDs().AsArray()
+	crds := crd.NewInitializer().AsArray()


I like how package prefix already tells the reader what is being initialized here! 👍

zen-dog · 2019-12-12T10:57:05Z

pkg/kudoctl/kudoinit/prereq/namespace.go

+	ns   *v1.Namespace
+}
+
+func newNamespaceSetup(options kudoinit.Options) KudoNamespace {


nit: simply newNamespace or newKudoNamespace. constructor methods are by convention named new + objectName

Good point, fixed that. I also made the KudoNamespace, KudoServiceAccount and KudoWebhook private, as they're only used internally in the prereq package

zen-dog · 2019-12-12T11:02:56Z

pkg/kudoctl/kudoinit/types.go

+	AsArray() []runtime.Object
+}
+
+// Options is the configurable options to init


I'd prefer types.go being about types only (no methods). Can we put Options and all methods in options.go? We can leave it in /kudoinit/options.go for now.

Yeah, I like that. Done

zen-dog · 2019-12-12T11:04:24Z

pkg/kudoctl/kudoinit/types.go

+	}
+}
+
+func GenerateLabels(labels map[string]string) map[string]string {


This would be a kudo_labels.go but I feel that @alenkacz would hate me for suggesting a new file for a two-liner :D

Yeah, i'm not too happy with this either. I thought about putting it in the Options object, but... it's not an option. I'll leave it here for now, maybe we'll find a good place later on when the init process grows.

zen-dog · 2019-12-12T11:08:36Z

pkg/kudoctl/kudoinit/prereq/webhook.go

-// installInstanceValidatingWebhook applies kubernetes resources related to the webhook to the cluster
-func installInstanceValidatingWebhook(client kubernetes.Interface, dynamicClient dynamic.Interface, ns string) error {
-	if err := installUnstructured(dynamicClient, certificate(ns)); err != nil {
+// Ensure IF is implemented


Suggested change

// Ensure IF is implemented

// Ensure kudoinit.InitStep interface is implemented

Refactor init code into setup package

000914c

ANeumann82 requested review from alenkacz, gerred, kensipe, nfnt and zen-dog as code owners December 10, 2019 16:55

alenkacz requested changes Dec 11, 2019

View reviewed changes

Fixed integration test

d07c147

Fixed unset options, improved error messages

950f1b9

ANeumann82 commented Dec 11, 2019

View reviewed changes

pkg/kudoctl/setup/prereq_serviceaccount.go Outdated Show resolved Hide resolved

ANeumann82 commented Dec 11, 2019

View reviewed changes

Reordered prerequisite init for ServiceAccount and RoleBindings

9c4e5bc

Fixed integration test that expected the custom SA and RB manifests in yaml output #1165

Fixed integration test expected messages

4b6ea2d

Ensure interfaces are implemented correctly

a7b24e7

ANeumann82 added 2 commits December 11, 2019 16:10

Next round of refactoring and reorganizing

1fccba8

Merge branch 'master' into an/refactor-init-setup

42aad64

# Conflicts: # pkg/kudoctl/cmd/init.go # pkg/kudoctl/cmd/init/prereqs.go

ANeumann82 added 2 commits December 11, 2019 17:22

Fixed yaml serialization

aa6aa42

Fixed yaml serialization

4ac0052

Dropped "prereq_" prefix from filenames

b606a8d

alenkacz approved these changes Dec 11, 2019

View reviewed changes

zen-dog approved these changes Dec 12, 2019

View reviewed changes

ANeumann82 added 2 commits December 12, 2019 12:56

Applied code review requests

3cdef43

Merge branch 'master' into an/refactor-init-setup

e6ac91b

ANeumann82 merged commit bbbdd4a into master Dec 13, 2019

ANeumann82 deleted the an/refactor-init-setup branch December 13, 2019 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor init code into setup package #1161

Refactor init code into setup package #1161

ANeumann82 commented Dec 10, 2019

alenkacz left a comment

alenkacz Dec 11, 2019

ANeumann82 commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

alenkacz commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

ANeumann82 Dec 11, 2019

ANeumann82 Dec 11, 2019

alenkacz commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

zen-dog commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

zen-dog commented Dec 11, 2019

alenkacz commented Dec 11, 2019

alenkacz left a comment

zen-dog left a comment

zen-dog Dec 12, 2019

zen-dog Dec 12, 2019

ANeumann82 Dec 12, 2019

zen-dog Dec 12, 2019

ANeumann82 Dec 12, 2019

zen-dog Dec 12, 2019

ANeumann82 Dec 12, 2019

zen-dog Dec 12, 2019

ANeumann82 Dec 12, 2019

	// Ensure IF is implemented
	// Ensure kudoinit.InitStep interface is implemented

Refactor init code into setup package #1161

Refactor init code into setup package #1161

Conversation

ANeumann82 commented Dec 10, 2019

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ANeumann82 commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

alenkacz commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

zen-dog commented Dec 11, 2019

ANeumann82 commented Dec 11, 2019

zen-dog commented Dec 11, 2019

alenkacz commented Dec 11, 2019

alenkacz left a comment

Choose a reason for hiding this comment

zen-dog 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