Refactor operator package installation #1542
Conversation
The old 'InstallPackage' function has been extracted into a separate package. It's functionality has been split up into multiple functions handling different installation resources. The function signature of 'install.Package' introduces variadic option parameters to provide a backwards-compatible API. Signed-off-by: Jan Schlicht <jan@d2iq.com>
nfnt
force-pushed
the
nfnt/refactor-package-install
branch
from
aaa183d to
de69d7f
Compare
| return nil | ||
| } | ||
|
|
||
| if err := validateParameters( |
There was a problem hiding this comment.
Shouldn't we validate parameters before we jump out in L83
There was a problem hiding this comment.
Parameter validation is only important for instances, that's why it's only done when actually installing an instance.
There was a problem hiding this comment.
In case of KEP-29, we're allowing potentially broken parameters to skip the validation and make it to the server
There was a problem hiding this comment.
The old code skipped this validation if no instance was to be installed. I moved it to run as early as possible now with the reasons you mentioned.
| } | ||
| } | ||
|
|
||
| return nil |
| "github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo" | ||
| ) | ||
|
|
||
| type Options struct { |
There was a problem hiding this comment.
I'd like to adopt the type.go-pattern as with other packages and keep all general types in an install/types.go. It keeps entangling cyclic dependencies later much easier.
There was a problem hiding this comment.
Adding a install.types.go won't change anything regarding cyclic dependencies, because it would still be in the same package. For this it would need to be in types.go of the packages package. I consider this an anti-pattern, types should be declared close to the functions using this type. Cyclic dependencies can be resolved by good package management following SRP.
There was a problem hiding this comment.
Adding a install.types.go won't change anything regarding cyclic dependencies
not yet, that's why I wrote that it becomes easier "later" 😉 I would only welcome it if you move other files into their own sub-packages.
I consider this an anti-pattern
well, k8s codebase disagrees with you, and frankly so do I. we adopted this pattern for a good reason. Lack of fine-grained imports in Go makes it challenging to avoid cyclic dependencies.
There was a problem hiding this comment.
I have yet to see where the Kubernetes code base disagrees. Their usage of types.go is done to provide consistency when using generators like deepcopy-gen or other kubebuilder tools on structures that are supposed to be used as part of the Kubernetes API. Which isn't the case here.
There was a problem hiding this comment.
The can be multiple reasons for that. And again, being forced to resolve multiple cyclic dependencies (and giving up on at least one of them) in our codebase makes me appreciate this pattern a lot. Especially in a package like install which is potentially used by server and client and itself uses server types (like Instance). But I guess we'll have to agree to disagree here.
| "github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo" | ||
| ) | ||
|
|
||
| func installOperatorAndOperatorVersion(client *kudo.Client, resources packages.Resources) error { |
There was a problem hiding this comment.
Nice, we'll need this method for the dependencies.
* Easier option handling * Improved logging Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
What this PR does / why we need it:
The old
InstallPackagefunction has been extracted into a separate package. It's functionality has been split up into multiple functions handling different installation resources. The function signature ofinstall.Packageintroduces variadic option parameters to provide a backward-compatible API.In preparation for #1514