Refactor functions to install/upgrade package resources into 'kudo' package #991

nfnt · 2019-10-23T12:40:44Z

What this PR does / why we need it:
This reduces the coupling between the respective commands and the actual
implementation that is taking care of these resources. It also allows non-CLI parts
to use this functionality.

Additional care has been taken to provide consistent error logging in Install and Upgrade.

zen-dog · 2019-10-23T16:04:08Z

As discussed extensively offline, crds is a highly misleading name for a pkg and should be avoided (also, this PRs title and description ;) )

…ces' package This reduces the coupling between the respective commands and the actual implementation that is taking care of these CRDs. It also allows non-CLI parts to use this functionality.

alenkacz

Overall I like it, have just few small comments:

first I think this package deserves a doc.go since we're planning that as an entrypoing for addons integration
I don't think that majority of the usages of %w is correct based on https://blog.golang.org/go1.13-errors - do we really want to expose the underlying wrapped kubernetes client errors?

pkg/kudoctl/cmd/install/install_test.go

alenkacz · 2019-10-24T08:15:46Z

pkg/kudoctl/cmd/upgrade.go

@@ -8,9 +8,8 @@ import (
 	"github.com/kudobuilder/kudo/pkg/kudoctl/env"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"
-	util "github.com/kudobuilder/kudo/pkg/util/kudo"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/util/resources"


could we move it one level up so it's pkg/kudoctl/util/resources ?

Overall I agree, this shouldn't be under pkg/kudoctl/util. But so shouldn't pkg/kudoctl/util/kudo. As the function of these packages is to abstract the usage of clientset they all should live in a folder in pkg and that folder shouldn't be named util 😄
But given that resources provides helper for the kudo package, I'd rather keep it close to that package for now, hence keep it in pkg/kudoctl/util. Instead we should move all of util as part of a separate PR.

There is this gopher talk from Ashley McNamara on package organization where he adamant on util packages being an anti-pattern. If anything, I'd try to reduce the util pkg footprint by moving things where they're used and name them accordingly.

Let's consolidate the footprint in this PR so it's focused on the new package, and then we can immediately open a refactoring that moves util out and gets rid of it entirely, wdyt @zen-dog?

pkg/kudoctl/util/resources/install.go

alenkacz · 2019-10-24T08:25:15Z

pkg/kudoctl/util/resources/install.go

+		return err
+	}
+
+	if err := kc.ValidateServerForOperator(resources.Operator); err != nil {


could we rename this to something like "ValidateServerVersionsForOperator"? :) I really had to go to the implementation to actually understand what this does

I know you did not introduce it but I just found it in your diff so we should probably improve :)

Let's tackle this in a different PR where we move this package and also improve on the function names, as mentioned in the comments above. wdyt?

pkg/kudoctl/util/resources/install.go

It's "resources", not "CRDs".

zen-dog · 2019-10-24T09:15:05Z

pkg/kudoctl/util/resources/package.go

+// - an operator name in the remote repository
+// in that order. Should there exist a local folder e.g. `cassandra` it will take precedence
+// over the remote repository package with the same name.
+func GetPackageResources(operatorName string, version string, repository repo.Repository) (*packages.PackageCRDs, error) {


Why is this method not part of the /packages pkg? It seems like a better place for it since it is dealing with packaged files (either locally or over the wire).

Also, naming:

Get-prefix is also considered an anti-pattern GetPackageResources() -> packages.Resources()

This method should also return *packages.Resources instead of PackageCRDs

Unfortunately, this can't be moved into packages, because this will result in import cycles due to circular package references. This is due to tight coupling in this as well as other packages. Further refactorings are necessary to clean this up and are out of scope of this PR. I'll move this to the kudo package for now.

pkg/kudoctl/util/resources/upgrade.go

zen-dog

Great effort, but we need to polish the resulting structure a bit more

pkg/kudoctl/util/resources/install.go

gerred · 2019-10-24T11:09:33Z

Commenting without explicit approval or requesting changes. This looks good and is heading the right direction, most of my feedback is echoing and not adding more on existing feedback.

alenkacz

I think this is a better version of what we had before so let's land it! 🚢

pkg/kudoctl/packages/package_test.go

alenkacz · 2019-10-25T09:52:55Z

pkg/kudoctl/util/kudo/resources.go

+// - an operator name in the remote repository
+// in that order. Should there exist a local folder e.g. `cassandra` it will take precedence
+// over the remote repository package with the same name.
+// TODO: move this to the 'packages' package -- currently not possible because if a dependency cycle


is this todo still relevant? I don't think so...

Yes, very relevant. This function should be in packages but can't be there without further refactorings, see #991 (comment)

okay then I would probably not move it here, since we want to move it somewhere else anyway 🤷‍♀

We should probably reorganize packages pgk similar to what we did for the engine pkg:

have packages/types.go with all package types

put everything else into packages/* sub-packages to avoid circular dependencies

This is the pattern that K8S uses a lot and it seems to make a lot of sense.

pkg/kudoctl/cmd/install/install.go

pkg/kudoctl/util/kudo/install.go

nfnt · 2019-10-25T12:51:50Z

Created #1002 to track further refactorings as discussed in this PR.

nfnt · 2019-10-25T13:34:33Z

Interesting linting error due to a circular dependency:

ERRO Running error: context loading failed: failed to load program with go/packages: internal error: go list gives conflicting information for package github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo [github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo.test] 
make: *** [Makefile:46: lint] Error 3

Let me fix this.

'pkg/kudoctl/cmd/env' depends on 'pkg/kudoctl/cmd/util/kudo' to implement a helper function that probably shouldn't live in this package.

zen-dog

Nice! 🚢

…ackage (#991) This reduces the coupling between the respective commands and the actual implementation that is taking care of these resources. It also allows non-CLI parts to use this functionality.

nfnt added kind/cleanup kind/enhancement labels Oct 23, 2019

nfnt requested review from alenkacz, gerred, kensipe and zen-dog as code owners October 23, 2019 12:40

nfnt self-assigned this Oct 23, 2019

nfnt changed the title ~~Refactor functions to install/upgrade CRDs into separate 'crds' package~~ Refactor functions to install/upgrade package resources into separate 'resources' package Oct 23, 2019

Jan Schlicht added 3 commits October 23, 2019 18:19

Refactor functions to install/upgrade resources into separate 'resour…

4caf540

…ces' package This reduces the coupling between the respective commands and the actual implementation that is taking care of these CRDs. It also allows non-CLI parts to use this functionality.

Add parameter handling to 'Install'

7665411

Rename package

867ee66

nfnt force-pushed the nfnt/refactor-package-functions branch from 42140a8 to 867ee66 Compare October 23, 2019 16:19

alenkacz reviewed Oct 24, 2019

View reviewed changes

Jan Schlicht added 2 commits October 24, 2019 10:47

Fix wrong naming

41cbfea

It's "resources", not "CRDs".

Don't wrap Kubernetes error types

e0fd86c

zen-dog reviewed Oct 24, 2019

View reviewed changes

pkg/kudoctl/util/resources/upgrade.go Outdated Show resolved Hide resolved

zen-dog requested changes Oct 24, 2019

View reviewed changes

pkg/kudoctl/util/resources/install.go Outdated Show resolved Hide resolved

Jan Schlicht added 2 commits October 24, 2019 13:06

Rename 'PackageCRDs' to 'Resources'

409661b

Move functions to 'kudo' package

b36f8bb

nfnt changed the title ~~Refactor functions to install/upgrade package resources into separate 'resources' package~~ Refactor functions to install/upgrade package resources into 'kudo' package Oct 24, 2019

Add a TODO to move 'Resources' to the 'packages' package

4d1760d

nfnt requested review from alenkacz and zen-dog October 25, 2019 09:03

alenkacz approved these changes Oct 25, 2019

View reviewed changes

Jan Schlicht added 2 commits October 25, 2019 12:11

Fix a typo

bec1df6

Change function name

55dc5e1

zen-dog reviewed Oct 25, 2019

View reviewed changes

pkg/kudoctl/cmd/install/install.go Outdated Show resolved Hide resolved

zen-dog reviewed Oct 25, 2019

View reviewed changes

pkg/kudoctl/util/kudo/install.go Outdated Show resolved Hide resolved

Move install/upgrade tests to 'kudo' package

a732659

nfnt requested a review from zen-dog October 25, 2019 11:40

Jan Schlicht added 3 commits October 25, 2019 15:28

Rename function to 'InstallPackage'

3183752

Merge branch 'master' into nfnt/refactor-package-functions

19a5cfe

go mod tidy

29ed505

Remove a circular dependency

459eadc

'pkg/kudoctl/cmd/env' depends on 'pkg/kudoctl/cmd/util/kudo' to implement a helper function that probably shouldn't live in this package.

zen-dog approved these changes Oct 25, 2019

View reviewed changes

nfnt merged commit b7f16ff into master Oct 25, 2019

nfnt deleted the nfnt/refactor-package-functions branch October 25, 2019 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor functions to install/upgrade package resources into 'kudo' package #991

Refactor functions to install/upgrade package resources into 'kudo' package #991

nfnt commented Oct 23, 2019 •

edited

Loading

zen-dog commented Oct 23, 2019

alenkacz left a comment

alenkacz Oct 24, 2019

nfnt Oct 24, 2019 •

edited

Loading

zen-dog Oct 24, 2019

gerred Oct 24, 2019

alenkacz Oct 24, 2019

nfnt Oct 25, 2019

zen-dog Oct 24, 2019

gerred Oct 24, 2019

nfnt Oct 24, 2019

zen-dog left a comment

gerred commented Oct 24, 2019

alenkacz left a comment

alenkacz Oct 25, 2019

nfnt Oct 25, 2019

alenkacz Oct 25, 2019

zen-dog Oct 25, 2019

nfnt commented Oct 25, 2019

nfnt commented Oct 25, 2019 •

edited

Loading

zen-dog left a comment

Refactor functions to install/upgrade package resources into 'kudo' package #991

Refactor functions to install/upgrade package resources into 'kudo' package #991

Conversation

nfnt commented Oct 23, 2019 • edited Loading

zen-dog commented Oct 23, 2019

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfnt Oct 24, 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

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

gerred commented Oct 24, 2019

alenkacz 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

nfnt commented Oct 25, 2019

nfnt commented Oct 25, 2019 • edited Loading

zen-dog left a comment

Choose a reason for hiding this comment

nfnt commented Oct 23, 2019 •

edited

Loading

nfnt Oct 24, 2019 •

edited

Loading

nfnt commented Oct 25, 2019 •

edited

Loading