Bundle Feature: Create Package Bundle #677

kensipe · 2019-08-01T02:21:14Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:
/component kudoctl
/component operator
/kind feature

What this PR does / why we need it:
This adds 1 feature identified in draft kep-15. It bundles a package into a tarball. It provides a small amount of validation in that the operator and params file must be present. We should add linting at some point. It allows for an output location and opt-in overwrite. It creates bundles based on the expected naming convention which is <operator.name>-<operator-version>.tgz.
It provides error handling around valid destination, valid operator folder and tgz exists.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kudo bundle <operator folder> creates a tgz file for the operator named based on <operator.name>-<operator-version>.tgz

alenkacz

I like the direction 👏 I did not understand the need for this command but now I see that we need it as a first step for the uploads!!!

I had few mostly code-readability questions and I think we need more tests and also docs for this command in cli documentation 🙈 I forgot about those for my command as well

pkg/kudoctl/bundle/bundle.go

pkg/kudoctl/cmd/bundle.go

pkg/kudoctl/bundle/bundle.go

pkg/kudoctl/cmd/bundle.go

pkg/kudoctl/cmd/bundle_test.go

pkg/kudoctl/bundle/bundle.go

pkg/kudoctl/cmd/bundle.go

kensipe · 2019-08-02T23:23:16Z

Failure

--- FAIL: TestTableNewBundleCmd (0.00s)
    package_test.go:51: 
        	Error Trace:	package_test.go:51
        	Error:      	Not equal: 
        	            	expected: ""
        	            	actual  : "invalid operator in path: /opt/first-operator"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-
        	            	+invalid operator in path: /opt/first-operator
        	Test:       	TestTableNewBundleCmd

This works locally... perhaps there is a difference or bug in the linux impl of afero.. need to investigate.

kensipe · 2019-08-05T14:29:19Z

The issue was a missed afero call... oops

Adding package cmd to cli docs

testing with in mem fs code clean up missed afero call for fromFolder

…the current test

alenkacz

As I don't understand the value of afero in production code and it looks like Ken discussed it with Gerred I am stepping away from the review as Gerred probably has more context...

alenkacz · 2019-08-06T12:12:49Z

I don't know how to dismiss my review :( I think someone else would have to do it

alenkacz

Okay I did one more pass and it actually looks ready to go from my point of view - I would personally just not drag afero into the code as we can test even without it. Other than that 👍

pkg/kudoctl/cmd/package_test.go

alenkacz · 2019-08-06T12:19:45Z

pkg/kudoctl/cmd/package_test.go

+	files.CopyOperatorToFs(fs, "../../../config/samples/first-operator", "/opt")
+	for _, test := range bundleCmdArgs {
+		newCmdBundle := newPackageCmd(fs, os.Stdout)
+		err := newCmdBundle.RunE(newCmdBundle, test.arg)


I feel like we don't necessarily need this test as we're testing creating tarballs as well as ToTarBundle the only method that the command is calling, I would probably as a next level just create integration test. Then we also don't need afero in kudoctl

the test above this tests unhappy paths... this one tests happy path. There is value in check the execution path to the successful creation of a tarball. It is easy to imagine more cases as we add more... should as a linter etc.

I am just not a big fan of dragging filesystem abstraction through the whole code path as I think this test tests pretty much the same thing as tar_test.go - I mean I feel like we can test the fact that we know how to create tarball from folder even on a lower level (to be honest, that should ideally be a library function, we should not ideally test it at all as this should be already provided to us). All other cases except the last one can be tested without afero because we can read normally from filesystem.

I am not blocking this review though, let's get another review - I am fine with the majority

I actually never heard why we dismissed my idea with passing in tarball creation as a function https://github.com/kudobuilder/kudo/pull/696/files#diff-70d20aa07496aa02fab75ab38ad29a6cR87 and then easily mocking it like this https://github.com/kudobuilder/kudo/pull/696/files#diff-536de88b39328683a008802ef51fcfb0R10 ... I find that much more elegant solution than having abstraction over FS everywhere...

alenkacz · 2019-08-06T12:21:08Z

pkg/kudoctl/files/file.go

+}
+
+// Sha256Sum calculates and returns the sha256 checksum
+func Sha256Sum(r io.Reader) (string, error) {


since we're right now using this only in tests, maybe this method could be inlined in the test as private?

that is my general inclination to make things private until needed.. this felt like a utility that should be available for future needs... there is more file work to come.

I think this is premature - I usually tend to extract when that case really happens because I know that intuition in this many times fail :) but 🤷‍♀

alenkacz

I am not blocking this, I am just not sure about the need of afero in production code... :/ Let's get another review, I am fine with the majority...

cmd/kubectl-kudo/main.go

alenkacz · 2019-08-06T13:52:24Z

pkg/kudoctl/cmd/package_test.go

+	files.CopyOperatorToFs(fs, "../../../config/samples/first-operator", "/opt")
+	for _, test := range bundleCmdArgs {
+		newCmdBundle := newPackageCmd(fs, os.Stdout)
+		err := newCmdBundle.RunE(newCmdBundle, test.arg)


I am just not a big fan of dragging filesystem abstraction through the whole code path as I think this test tests pretty much the same thing as tar_test.go - I mean I feel like we can test the fact that we know how to create tarball from folder even on a lower level (to be honest, that should ideally be a library function, we should not ideally test it at all as this should be already provided to us). All other cases except the last one can be tested without afero because we can read normally from filesystem.

I am not blocking this review though, let's get another review - I am fine with the majority

alenkacz

I feel like having afero in production code or not is solely technical decision and we should easily be able to have conversation about what are the pros and cons of bringing that in and if it's worth it. I think in current form it it not, since I am sure we can achieve the same code coverage without afero as right now we're just using it to assert that command returns no errors. Which I achieved with no additional dependencies here https://github.com/kudobuilder/kudo/pull/696/files#diff-536de88b39328683a008802ef51fcfb0R10 Actually never heard why that is worse option.

But I won't be blocking this, it looks like removing afero is a no go for you...

alenkacz · 2019-08-07T07:37:12Z

pkg/kudoctl/cmd/package.go

+		kubectl kudo package ../operators/repository/zookeeper/operator/ --destination=out-folder`
+)
+
+type packageCmd struct {


this should be named options to be in line with other commands

I much prefer the cli model that helm uses... which aligns with this style. I plan to refactor the other commands to align with this one.

should we maybe agree on that first? 😄 I kind of like the current shape which is what kubectl and other CLIs does...

we have a lot of work to do in the CLI when it comes to naming... for instance defaultOptions https://github.com/kudobuilder/kudo/blob/master/pkg/kudoctl/cmd/upgrade.go#L42
makes no sense... as this var lives in the package space and is likely the upgradeOptions or defaultUpgradeOptions.

Oh yeah, defaultOptions for sure, that should be renamed. But I like the consistency of having those options classes for each command...

pkg/kudoctl/cmd/package.go

pkg/kudoctl/cmd/package_test.go

alenkacz · 2019-08-07T07:42:32Z

pkg/kudoctl/cmd/package_test.go

+	files.CopyOperatorToFs(fs, "../../../config/samples/first-operator", "/opt")
+	for _, test := range bundleCmdArgs {
+		newCmdBundle := newPackageCmd(fs, os.Stdout)
+		err := newCmdBundle.RunE(newCmdBundle, test.arg)


I actually never heard why we dismissed my idea with passing in tarball creation as a function https://github.com/kudobuilder/kudo/pull/696/files#diff-70d20aa07496aa02fab75ab38ad29a6cR87 and then easily mocking it like this https://github.com/kudobuilder/kudo/pull/696/files#diff-536de88b39328683a008802ef51fcfb0R10 ... I find that much more elegant solution than having abstraction over FS everywhere...

alenkacz · 2019-08-07T07:44:08Z

pkg/kudoctl/files/file.go

+}
+
+// Sha256Sum calculates and returns the sha256 checksum
+func Sha256Sum(r io.Reader) (string, error) {


I think this is premature - I usually tend to extract when that case really happens because I know that intuition in this many times fail :) but 🤷‍♀

zen-dog

This seems like a great addition to KUDO. I left/upvoted a few questions/suggestions so please take a look. Otherwise, good work!

pkg/kudoctl/bundle/bundle.go

zen-dog · 2019-08-07T12:04:45Z

pkg/kudoctl/bundle/bundle.go

+
+// getFullPathToTarget takes destination path and file name and provides a clean full path while ensure the file does not exist.
+func getFullPathToTarget(fs afero.Fs, destination string, name string, overwrite bool) (string, error) {
+	if destination == "." {


This whole path parsing of the destination looks brittle to me. Why don't you just resolve destination to an absolute path with os.Abs(destination) and then use the result with fs?

I've been thinking of moving in that direction as well. I have another branch based on this PR. I will refactor it there. great feedback!

zen-dog · 2019-08-07T12:24:21Z

pkg/kudoctl/bundle/tar.go

+
+// Untar takes a destination path and a reader; a tar reader loops over the tarfile
+// creating the file structure at 'path' along the way, and writing any files
+func Untar(fs afero.Fs, path string, r io.Reader) error {


This method is practically the same as parseTarPackage in the above bundle.go, except that the above already parses files into PackageFiles struct. Do we really need the duplication? Especially because this method is only used once in the test? Do we need it at all? And if we do, could we consolidate both methods?

interesting point... I think we could create a untar(reader, writer) where the write function is to the fs or is a function or object to create a package... I'll capture that and move forward.

zen-dog · 2019-08-07T12:26:09Z

pkg/kudoctl/bundle/tar_test.go

+
+	Untar(fs, "/opt/untar", f)
+
+	u, _ := os.Open("/opt/untar/operator.yaml")


Variable names like u, o, f? Is it really you, @kensipe? 😂 Or is idiomatic go slowly getting to you?

pkg/kudoctl/cmd/package.go

pkg/kudoctl/cmd/package_test.go

zen-dog · 2019-08-07T13:32:28Z

pkg/kudoctl/files/file.go

+
+// CopyOperatorToFs used with afero usually for tests to copy files into a filesystem.
+// copy from local file system into in mem
+func CopyOperatorToFs(fs afero.Fs, opath string, base string) {


This method looks like a general-purpose file method but it doesn't handle recursive directories, symlinks and what not. Wdyt about "steeling" this method from docker? Or make it test-private as Alena suggested.

I'm not sure about stealing... I'll look. It was meant to be enough to run the tests needed. I already have tests for future PR needing it... hence not private.

Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

jbarrick-mesosphere · 2019-08-08T02:43:19Z

This looks good, but where did we land as far as "bundle" versus "package"? I see both here, but I don't think it's clarified what the difference is.

jbarrick-mesosphere · 2019-08-08T02:43:56Z

/approve

kudo-ci · 2019-08-08T02:44:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alenkacz, jbarrick-mesosphere, kensipe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alenkacz,jbarrick-mesosphere,kensipe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kensipe · 2019-08-08T16:38:58Z

merged in master to resolve the conflict

kensipe requested review from alenkacz and fabianbaier as code owners August 1, 2019 02:21

kudo-ci added the kind/feature label Aug 1, 2019

kudo-ci requested review from guenter and jbarrick-mesosphere August 1, 2019 02:21

kudo-ci added approved size/L labels Aug 1, 2019

kensipe force-pushed the ken/bundler branch from 91a7692 to 2d2bb3d Compare August 1, 2019 02:22

alenkacz requested changes Aug 1, 2019

View reviewed changes

kudo-ci added the needs-rebase label Aug 1, 2019

kensipe force-pushed the ken/bundler branch from ff302e5 to 7377e54 Compare August 1, 2019 16:21

kudo-ci removed the needs-rebase label Aug 1, 2019

kensipe force-pushed the ken/bundler branch from f1e8bf1 to e8a624d Compare August 1, 2019 20:40

kensipe requested a review from gerred as a code owner August 2, 2019 20:21

kensipe force-pushed the ken/bundler branch from 9e7f87d to c4bb85b Compare August 2, 2019 23:07

kensipe added 3 commits August 5, 2019 15:16

Feature package command

9c7a6ab

Adding package cmd to cli docs

add afero as a file system abstraction. Tests currently still use OsFs

7d7b21a

testing with in mem fs code clean up missed afero call for fromFolder

adding in checks for tarball creation

bb13f23

kensipe force-pushed the ken/bundler branch from 0e79b91 to bb13f23 Compare August 5, 2019 20:17

kensipe added 3 commits August 5, 2019 15:26

happy linter.. happy life

3ce5768

tar test written for in-mem

154cbfa

hash on gzip is problematic, tar and untar with a check of a file is …

e6042ad

…the current test

kudo-ci added size/XL and removed size/L labels Aug 5, 2019

alenkacz reviewed Aug 6, 2019

View reviewed changes

alenkacz requested changes Aug 6, 2019

View reviewed changes

kensipe requested a review from alenkacz August 6, 2019 13:13

kensipe added 2 commits August 6, 2019 08:31

switched to using testdata for package cmd test

be0c21a

linter needs to be my precondition of git push

2058b09

kensipe mentioned this pull request Aug 6, 2019

KEP: Draft For Repository (targeted for KEP-15) #654

Merged

alenkacz reviewed Aug 6, 2019

View reviewed changes

kensipe added 2 commits August 6, 2019 09:17

reducing visiblity of fs

62f2564

tarball code needs to use tarball writer

d6b9090

kudo-ci added size/L and removed size/XL labels Aug 6, 2019

move afero out of main and into cmd package

7f0634b

alenkacz approved these changes Aug 7, 2019

View reviewed changes

zen-dog reviewed Aug 7, 2019

View reviewed changes

kensipe and others added 3 commits August 7, 2019 09:35

Apply suggestions from code review

615a323

Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

updates based on pr feedback

b9b1ec4

adding details based on PR feedback

d2d76a5

Merge branch 'master' into ken/bundler

a9a1b9a

kensipe merged commit e94ec41 into master Aug 8, 2019

kudo-ci deleted the ken/bundler branch August 8, 2019 17:04


		Untar(fs, "/opt/untar", f)

		u, _ := os.Open("/opt/untar/operator.yaml")

Bundle Feature: Create Package Bundle #677

Bundle Feature: Create Package Bundle #677

Conversation

kensipe commented Aug 1, 2019

alenkacz left a comment

Choose a reason for hiding this comment

kensipe commented Aug 2, 2019

kensipe commented Aug 5, 2019

alenkacz left a comment • edited Loading

Choose a reason for hiding this comment

alenkacz commented Aug 6, 2019

alenkacz left a comment • 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

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

alenkacz Aug 7, 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

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

jbarrick-mesosphere commented Aug 8, 2019

jbarrick-mesosphere commented Aug 8, 2019

kudo-ci commented Aug 8, 2019

kensipe commented Aug 8, 2019

alenkacz left a comment •

edited

Loading

alenkacz left a comment •

edited

Loading

alenkacz Aug 7, 2019 •

edited

Loading