-
Notifications
You must be signed in to change notification settings - Fork 103
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
Support new KEP-9 package format in installs #343
Conversation
pkg/kudoctl/util/repo/package.go
Outdated
GetInstallCRDs() (*InstallCRDs, error) | ||
} | ||
|
||
type V0Package struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as much as I love my interface here we can drop V0 packages immediately, up for discussion. Potentially we could drop it when we convert our official repo to the new format? But we cannot do that until just before 0.3.0 release, right? 🤔
pkg/kudoctl/util/repo/package.go
Outdated
|
||
const apiVersion = "kudo.k8s.io/v1alpha1" | ||
|
||
type InstallCRDs struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstallCRDs is kind of a weird name for a type in my mind for some reason. I know it's technically right and I can't think of a better one, just wanted to observe that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
type InstallCRDs struct { | |
type BundleCRDs struct { |
or
type InstallCRDs struct { | |
type BundleV1CRDs struct { |
or
type InstallCRDs struct { | |
type V1Alpha1CRDs struct { |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also dislike the InstallCRDs
name. It's CRDs now, but I'm sure we'll have more file types at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with PackageCRDs
- we can always change that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just don't forget all the parameters and methods still named *InstallCRDs
;)
Co-Authored-By: Aleksey Dukhovniy <[email protected]>
pkg/kudoctl/util/repo/package.go
Outdated
} | ||
|
||
// ReadFileSystemPackage reads package from filesystem and converts it to the CRD format | ||
func ReadFileSystemPackage(path string) (*InstallCRDs, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is technically not needed in this PR but I implemented it to ease the testing in the beginning and now since it's tested I decided to keep it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need to make a return visit... I want to comb thru the tests better.
nice work @alenkacz
} | ||
|
||
func fromFilesystem(packagePath string) (*v1Package, error) { | ||
result := newV1Package() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize "package" is not ok for a var name... but not a fan of result... that could be anything and true for any result. thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no strong opinion but I tend to use result for variables containing the return type of the whole function, so the result of that function. I am open to changing that.
if err != nil { | ||
return nil, err | ||
} | ||
return &result, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
questions that come to mind:
- is there any file size limits? or any validation other than the rules of marshaling?
(the concern is byte arrays and sizes) - there is no validation after the fact. There seems to be a happy path of all the files should be there. What if there is a framework file, but not other file? or template file and no framework file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We validate when we do getInstallCRDs
. You cannot really do it much earlier because some parts of the validation can be done only AFTER you load everything and you start parsing the loaded YAML files. E.g. to find out that you're missing a template file that is included inside the package. All that will be caught in getInstallCRDs
and it will return an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the size, we should at least validate the bundled package is not larger than 1MB (etcd's max key size). I know it's unlikely, but I'm not sure if we would get a sane/user understandable error back from Kubernetes.
if err != nil { | ||
return err | ||
} | ||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code seems to conflate file walking and file processing. It is reasonably easy to follow... but what do you think of having a filewalker that returns a set of string paths... and a processor that takes a set of string paths and processes them? In my mind it simplifies the reasoning and testing of the 2 concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, but the filewalker is actually what the library provides. If you return string paths then you basically need to iterate that tarball two times. I saw a lot of code snippets how people deal with tarballs in golang and everybody seems to be doing it this way...
|
||
tr := tar.NewReader(gzr) | ||
|
||
result := newV1Package() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same concern with var name "result"
if err != nil { | ||
return nil, err | ||
} | ||
return &result, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the size, we should at least validate the bundled package is not larger than 1MB (etcd's max key size). I know it's unlikely, but I'm not sure if we would get a sane/user understandable error back from Kubernetes.
pkg/kudoctl/util/repo/package.go
Outdated
|
||
const apiVersion = "kudo.k8s.io/v1alpha1" | ||
|
||
type InstallCRDs struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
type InstallCRDs struct { | |
type BundleCRDs struct { |
or
type InstallCRDs struct { | |
type BundleV1CRDs struct { |
or
type InstallCRDs struct { | |
type V1Alpha1CRDs struct { |
?
) | ||
|
||
const ( | ||
frameworkV1FileName = "framework.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
difference between V1
and v1alpha1
and v0.2.0
?
Does frameworkV1FileName
represent the new file structure from kep-9 and we call it V1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I can drop the V1 if it's confusing. Basically I called the old one v0 and the kep-p v1
@@ -48,7 +49,7 @@ require ( | |||
github.com/prometheus/common v0.2.0 // indirect | |||
github.com/prometheus/procfs v0.0.0-20190209105433-f8d8b3f739bd // indirect | |||
github.com/rogpeppe/go-internal v1.2.2 // indirect | |||
github.com/spf13/afero v1.2.1 // indirect | |||
github.com/spf13/afero v1.2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now we are using afero
just for our table tests in the package_test.go
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! PTAL at my comments.
pkg/kudoctl/cmd/install/install.go
Outdated
@@ -151,7 +151,7 @@ func installFramework(name, previous string, repository repo.FrameworkRepository | |||
|
|||
bundleName := bundleVersion.Name + "-" + bundleVersion.Version | |||
|
|||
bundle, err := repository.DownloadBundle(bundleName) | |||
crds, err := repository.DownloadBundle(bundleName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we rename this method to DownloadPackage
for consistency? Also, it doesn't just download but also untars and parses so maybe GetPackage
?
pkg/kudoctl/util/repo/package.go
Outdated
var bf bundle.Framework | ||
|
||
if err = yaml.Unmarshal(bytes, &bf); err != nil { | ||
return errors.Wrap(err, "cannot unmarshal framework") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return errors.Wrap(err, "cannot unmarshal framework") | |
return errors.Wrap(err, "failed to unmarshal framework file %v", file.Name()) |
pkg/kudoctl/util/repo/package.go
Outdated
} | ||
switch { | ||
case isFrameworkV1File(file.Name()): | ||
var bf bundle.Framework |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not unmarshaling directly to result.Framework
?
pkg/kudoctl/util/repo/package.go
Outdated
case isParametersV1File(file.Name()): | ||
var params map[string]map[string]string | ||
if err = yaml.Unmarshal(bytes, ¶ms); err != nil { | ||
return errors.Wrapf(err, "unmarshalling %s content", file.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return errors.Wrapf(err, "unmarshalling %s content", file.Name()) | |
return errors.Wrapf(err, "failed to unmarshal parameters file %s", file.Name()) |
} | ||
|
||
switch { | ||
case isFrameworkV1File(header.Name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is identical to the unmarshaling files from func fromFilesystem()
? We should extract it into a separate method.
instanceV0FileName = "-instance.yaml" | ||
) | ||
|
||
func TestReadFileSystemPackage(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests 👏
@@ -0,0 +1,13 @@ | |||
apiVersion: kudo.k8s.io/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe, the golden files in the go world have .golden
extension.
What type of PR is this?
/component kudoctl
What this PR does / why we need it:
kudo install
is able to install frameworks packages in the new KEP-9 style package format. The old format is no longer supported.Which issue(s) this PR fixes:
Fixes #349
Special notes for your reviewer:
Does this PR introduce a user-facing change?: