Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package resolvers now only returns resolved resources #1724

Merged
merged 6 commits into from Oct 21, 2020

Conversation

zen-dog
Copy link
Contributor

@zen-dog zen-dog commented Oct 20, 2020

Summary:
Package resolver interface used to return package.Package containing the underlying package files and the converted resources:

type Package struct {
	// transformed server view
	Resources *Resources
	// working with local package files
	Files *Files
}

The original assumption was that the files are always present. However, in some cases e.g. InClusterResolver all the resources are already installed in the cluster and there are no files that lead to some hacky workaround. Turns out, that files are not needed as most callers needed the resolved resources anyway.
This refactoring removes this entanglement and allows for better integration of already installed operators e.g. one could list parameters of the installed zookeeper operator with: kudo package list parameters zookeeper --in-cluster (not yet implemented).

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

Copy link
Member

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, two small nits, nothing serious

pkg/kudoctl/resources/dependencies/resolve_test.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/generate/maintainer.go Outdated Show resolved Hide resolved
Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like packages.Package is no longer used, let's remove it. Also, some comments still reference packages.Package, let's update them as well.

pkg/apis/kudo/v1beta1/parameter_types_helpers.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/generate/maintainer.go Outdated Show resolved Hide resolved
pkg/kudoctl/packages/reader/read_dir.go Outdated Show resolved Hide resolved
Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Don't forget to update the now obsolete comments around packages.Package in pkg/controller/instance/resolver_incluster.go and pkg/kudoctl/packages/resolver/resolver_incluster.go.

Summary:
Package resolver interface used to return `package.Package` containing the underlying package files and the converted resources:

```
type Package struct {
	// transformed server view
	Resources *Resources
	// working with local package files
	Files *Files
}
```

The original assumption was that the files are always present. However, in some cases e.g. `InClusterResolver` all the resources are already installed in the cluster and there are no files which lead to some hacky workaround. Turns out, that files are not needed as most callers needed the resolved resources anyway.
This refactoring removes this entanglement and allows for better integration of already installed operators e.g. one could list parameters of the installed zookeeper operator with: `kudo package list parameters zookeeper --in-cluster` (not yet implemented).

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
…anymore

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loving how this is cleaned up 👏

@zen-dog zen-dog merged commit 027e1d2 into main Oct 21, 2020
@zen-dog zen-dog deleted the ad/entangle-resolver branch October 21, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants