Add Outputs support to Docker Driver #82

astrieanna · 2019-07-25T21:10:56Z

In pursuit of cnabio/duffle#747
Fixes #14 and fixes #72

This PR:

In action, if the driver gives you output files, put them in the claim.
When adding output files to the claim, check that they match the definitions.
Add Output support in Docker driver, and integration tests for same.
Make K8s and Command drivers compile, but do not implement support.

cc @youreddy @jeremyrickard

jeremyrickard · 2019-07-25T21:28:42Z

🎊 thanks for the hard work @astrieanna @youreddy

driver/docker/docker_integration_test.go

driver/driver.go

driver/docker/docker.go

silvin-lubecki · 2019-07-26T13:35:11Z

driver/docker/docker.go

+	}
+	defer os.RemoveAll(outputsFolder)
+
+	// On Mac, the default temp folder (set by the $TMPDIR env variable) is in /var.


Thanks for spotting that, just added this feedback to the Docker Desktop Team, should be shortly fixed 👍

driver/docker/docker.go

action/uninstall_test.go

action/run_custom_test.go

action/install_test.go

action/action.go

silvin-lubecki

Thank you @astrieanna for this great PR 👍
I have just 2 things I think we need to fix:

the integration test requiring an image pushed on the hub
the BindMounting of the outputs which forbids running the invocation image on a remote engine

youreddy · 2019-07-29T21:29:29Z

@silvin-lubecki we just updated the PR branch and incorporated the suggestions you made. Let us know if you have any more feedback!

astrieanna · 2019-07-30T00:18:47Z

@glyn do you want to take a look at this side of the PR?

jeremyrickard · 2019-07-30T01:29:53Z

Looks like the tests are failing on the linter:

action/action.go:92:10: `if` block ends with a `return` statement, so drop this `else` and outdent its block (golint)
		} else {
		       ^
Makefile:25: recipe for target 'lint' failed

radu-matei · 2019-07-30T07:26:22Z

/brig run

driver/docker/docker_integration_test.go

radu-matei

Thanks a lot for implementing this, and for the thorough testing efforts.
Once we fix CI, LGTM.

This also closes #14

glyn · 2019-07-30T09:49:36Z

action/action.go

+// allowedTypes takes an output Schema and returns a map of the allowed types (to true)
+// and a string that is a list of all the allowed types (for error messagse)
+// or an error (if reading the allowed types from the schema failed).
+func allowedTypes(outputSchema definition.Schema) (map[string]bool, string, error) {


The idiomatic representation of a set of strings in Go is map[string]struct{} rather than map[string]bool.

What makes the map[string]struct{} representation idiomatic?

It looks like Effective Go approves of map[string]bool.

It's just common practice because struct{} occupies 0 bytes unlike bool. Also, the map[string]bool representation isn't ideal because there are three possibilities for any given string (absent, present and mapping to true, present and mapping to false) which isn't what you really want for a set.

I'm disappointed that Effective Go hasn't picked up on this.

I'm not worried about the memory allocation here. This is a set of allowed type names; there's a small a constant number of JSON types, so in practice this will be tiny.

I agree. I think the representation issue is more important, but I don't want to argue and it will be easy to switch the types later.

For the record, Dave Cheney also prefers map[T]bool as way of representing a set of T. I still prefer map[T]struct{}, but it's only a preference.

action/action.go

action/install_test.go

glyn · 2019-07-30T11:00:57Z

driver/docker/docker.go

+
+		var contents []byte
+		// CopyFromContainer strips prefix above outputs directory.
+		pathInContainer := filepath.Join("/cnab/app", header.Name)


I'm wondering about the behaviour in Windows. I think we'd end up with paths like "/cnab/app\x\y\z" which seems odd. Would it be cleaner to use paths as they were in the container? If so, just use path.Join. (Also, note that such paths are valid Windows paths FWIW.)

We gave this a try in the playground and couldn't find a difference between the two https://play.golang.org/p/tQVQNw1YJZR. Is there something we're missing?

The playground is running on *IX and so filepath.Join and path.Join give the same results. On Windows, the results would be different. (BTW if header.Name can have embedded back-slashes, these should be converted to forward slashes.)

If header.Name will have embedded back-slashes on Windows, another way to make pathInContainer consistent would be to use:

pathInContainer := filepath.Join("cnab", "app", header.Name)

It would be helpful if someone with a Windows system could try this PR and see how it handles outputs in multiple directories.

Presumably some of our friends at Microsoft could help us out with that :)
@vdice @jeremyrickard @radu-matei

I agree with @glyn about pathInContainer := filepath.Join("cnab", "app", header.Name) 👍

Based on @jeremyrickard's experiments on Windows and if the variable name pathInContainer (where we mean a Linux container) is to be taken literally, then the path separator should be / and so it's probably best to use path.Join instead of filepath.Join along these lines:

Suggested change

pathInContainer := filepath.Join("/cnab/app", header.Name)

pathInContainer := path.Join("cnab", "app", header.Name)

driver/docker/docker_integration_test.go

glyn · 2019-07-30T11:06:32Z

In pursuit of deislabs/duffle#747

This PR:

In action, if the driver gives you output files, put them in the claim.

When adding output files to the claim, check that they match the definitions.

Add Output support in Docker driver, and integration tests for same.

Make K8s and Command drivers compile, but do not implement support.

Why not? This PR will break those drivers. But at least could you raise a couple of issues to cover this and maybe reference them from TODOs in the code? We would then need to implement those issues before bumping duffle to use this PR.

Actually, I think it would be much better to make this PR cover all the drivers in case other issues come out of the woodwork during implementation.

astrieanna · 2019-07-30T18:22:45Z

In pursuit of deislabs/duffle#747

Make K8s and Command drivers compile, but do not implement support.

Why not? This PR will break those drivers. But at least could you raise a couple of issues to cover this and maybe reference them from TODOs in the code? We would then need to implement those issues before bumping duffle to use this PR.

Actually, I think it would be much better to make this PR cover all the drivers in case other issues come out of the woodwork during implementation.

The linked issue includes a summary of why adding support to the k8s driver was not implemented in this PR. There is already an issue for implementing support in the k8s driver. #73

The Command driver execs a process to run the invocation image. It is not clear what the expected interface for returning outputs would be for the executable the driver is calling. I have added an issue for implementing support in the command driver: #84

This PR touches a lot of code, which creates a greater risk for merge conflicts. It implements the outputs in one of the three drivers, which allows for testing the rest of the code changes. I would prefer to merge this PR sooner, while the community discusses options for implementing support in the other drivers.

jeremyrickard · 2019-07-30T22:15:44Z

follow up referenced above.

glyn · 2019-07-31T07:48:17Z

This PR touches a lot of code, which creates a greater risk for merge conflicts. It implements the outputs in one of the three drivers, which allows for testing the rest of the code changes. I would prefer to merge this PR sooner, while the community discusses options for implementing support in the other drivers.

Fair enough. After merging this PR, we'll need to implement #73 and #84 before we can bump cnab-go in duffle.

youreddy · 2019-07-31T18:02:56Z

This PR touches a lot of code, which creates a greater risk for merge conflicts. It implements the outputs in one of the three drivers, which allows for testing the rest of the code changes. I would prefer to merge this PR sooner, while the community discusses options for implementing support in the other drivers.

Fair enough. After merging this PR, we'll need to implement #73 and #84 before we can bump cnab-go in duffle.

@glyn I'm curious what your specific concerns are for bumping cnab-go before the other drivers are have output support. Are you worried about uncovering issues when implementing outputs in the other drivers or about the specific user experience regression in duffle?

jeremyrickard · 2019-07-31T20:07:41Z

After merging this PR, we'll need to implement #73 and #84 before we can bump cnab-go in duffle.

I don't think it's necessary to do that. This is adding functionality and not breaking the experience (i.e. outputs didn't exist before), seems like worthy of a a minor release for docker output functionality, then another minor release in duffle (and cnab-go) for k8s and command driver outputs.

vdice

Only had a few suggestions, none of which are blocking. LGTM. Great work!

action/action.go

action/uninstall.go

glyn · 2019-08-02T08:15:11Z

This PR touches a lot of code, which creates a greater risk for merge conflicts. It implements the outputs in one of the three drivers, which allows for testing the rest of the code changes. I would prefer to merge this PR sooner, while the community discusses options for implementing support in the other drivers.

Fair enough. After merging this PR, we'll need to implement #73 and #84 before we can bump cnab-go in duffle.

@glyn I'm curious what your specific concerns are for bumping cnab-go before the other drivers are have output support. Are you worried about uncovering issues when implementing outputs in the other drivers or about the specific user experience regression in duffle?

Since this PR only claims to get the other drivers to compile, I was concerned that their function might regress. If it leaves them usable but without output support, that's fine. /cc @jeremyrickard

astrieanna · 2019-08-05T08:34:17Z

@jeremyrickard On slack, it sounded like you might be our Windows tester?

silvin-lubecki · 2019-08-05T09:18:10Z

@astrieanna there's a conflict on action/action_test.go, can you rebase this PR? 🦁

astrieanna · 2019-08-05T09:38:07Z

@astrieanna there's a conflict on action/action_test.go, can you rebase this PR? 🦁

I've been holding off on rebasing until we've got a green-light to merge otherwise (to avoid resolving conflicts more than once). Is the rebase needed before testing the paths on windows?

driver/docker/docker.go

silvin-lubecki · 2019-08-05T09:54:17Z

driver/docker/docker.go

 		}
-		return fmt.Errorf("container exit code: %d", s.StatusCode)
+		opResult, _ := d.fetchOutputs(ctx, resp.ID)


Same here for the error dropped.

radu-matei · 2019-08-06T13:22:28Z

/brig run

glyn

LGTM once @jeremyrickard has tested on Windows to check https://github.com/deislabs/cnab-go/pull/82/files#r310527660

glyn

I'm approving conditional upon @jeremyrickard testing https://github.com/deislabs/cnab-go/pull/82/files#r310527660 on Windows. Since I'm going to be out until Monday, I don't want to hold this up artificially.

glyn

I think we need to use path.Join instead of filepath.Join to avoid inserting a \ into a (Linux) container path.

glyn

Approving conditional on the filepath->path change. I'm out until Monday and don't want to block this PR.

jeremyrickard · 2019-08-07T16:04:33Z

@youreddy @astrieanna sorry for the delay. I ran your test on my windows machine after fixing it:

filePath Join:  \cnab\app\outputs\x\y\z
justPath Join:  /cnab/app/outputs\x\y\z

I think that since we're assuming that we're reading files from a linux based path (/cnab/app/output) and we're populating that map with that path, I don't think filepath is appropriate due to the cross platform use case (windows machine -> linux docker image).

I think the more appropratie thing for now is

pathInContainer := path.Join("cnab", "app", header.Name)

We'll probably need to revisit with Windows containers, but lets not support that use case yet.

radu-matei · 2019-08-07T17:40:30Z

/brig run

youreddy · 2019-08-07T17:47:13Z

I just pushed up the path.Join and the error handling changes. I think the only thing left is @silvin-lubecki's request for building the image before or during the docker integration tests, which I'm working on now

radu-matei · 2019-08-07T19:15:19Z

/brig run

silvin-lubecki

Thank you @astrieanna and @youreddy LGTM 👍 👍

radu-matei · 2019-08-08T13:22:34Z

I think this needs a rebase, and it should be good to go?

Thanks a lot for the patience and work on this PR, @youreddy and @astrieanna!

- Add Outputs in Claim struct. - In action, if driver gives you output files, put them in the claim. - Add Output support in Docker driver. - Change container wait condiditon to WaitConditionNextExit in order to fetch outputs before the container terminates. - Add docker driver integration test and an example bundle. - Include build script for creating the image used in integration test. - Add `make integration-test` to simplify running unit and integration tests. - Make K8s and Command drivers compile, but do not implement support. - Handle tmpdir path weirdness with Docker for Mac. - Handle Output type(s). This does not implement Output support in the k8s driver. More information about the challenges to complete this effort can be found in cnabio/duffle#747. Signed-off-by: Leah Hanson <lhanson@pivotal.io>

technosophos · 2019-08-08T21:39:41Z

/brig run

astrieanna mentioned this pull request Jul 25, 2019

Add duffle claim show --output cnabio/duffle#815

Merged

jeremyrickard mentioned this pull request Jul 25, 2019

[spec compliance] Implement Outputs in Docker Driver #72

Closed