Feat: bundle outputs #438

vdice · 2019-07-01T23:20:58Z

This PR adds:

functionality to parse a new outputs section in a porter manifest (Note: the current implementation requires a mixin-specific step/action output to be declared before it can be exposed/used as a bundle-level output.)
functionality to place output values under /cnab/app/outputs in the execution environment, per the CNAB Spec (outputs are currently written only when added under the new outputs section and the optional applyTo specification doesn't conflict)
functionality to place these same outputs under the local/user's $PORTER_HOME/outputs/<bundle name> directory, assuming the default Docker driver is being used
cli command: porter bundle show to list a bundle's claim and outputs

TODO:

add a cli command that users can invoke to print outputs from the corresponding local file (question around AC posed in [Spec]: Support CNAB Bundle Outputs #344 (comment))
add unit tests for logic added in run.go
docker driver bind mount refactor/units
update the base porters schema template to include the outputs section
add/update docs

vdice · 2019-07-03T23:24:19Z

In the most recent commit, I went ahead and added the cli commands mentioned/proposed in #344 (comment)

vdice · 2019-07-08T16:42:36Z

Putting into 'ready for review' while I finish up unit tests...

carolynvs

I'm not done with the review but wanted to get you thinking about maybe moving the command into a different spot.

build/testdata/bundles/wordpress/porter.yaml

carolynvs · 2019-07-08T17:26:55Z

cmd/porter/outputs.go

+
+func buildOutputsCommand(p *porter.Porter) *cobra.Command {
+	cmd := &cobra.Command{
+		Use:     "outputs",


I think that this would be better not as a standalone command but as a task-based command that is used to inspect an installed bundle.

So instead of having porter outputs -b bundle name (which is off anyway because we are actually working with claim names right?), what if we had a command that let you look at an installed bundle and also had it dig deeper into additional available into (like outputs) if the bundle had it?

Maybe something like this:

$ porter bundles list NAME CREATED MODIFIED LAST ACTION LAST STATUS mysql 2019-06-24 2019-06-24 install success $ porter bundle show mysql Name mysql Created 2019-06-24 Modified 2019-06-24 Last Action install Last Status success Outputs ===================================================================== Name Type Value -----------+--------+--------------------------------------------------------+ hostName String wpmysql port Integer 5423 clientCert String /Users/carolynvs/.porter/outputs/.../mysql/clientCert

clientCert is writeOnly so we don't display it. Find some logic for not dumping sensitive or giant info

Roger; I can switch tactics and implement porter bundle show [NAME] to show bundle/claim deets, including outputs.

Should we leave this PR with that command or do we also want a solution/command for getting a particular output value? (Perhaps to merely echo/print or perhaps to send to a file, etc.)

If someone wants to just get a single output value, I would encourage them to use the --output json and then use jq or something to grab what they need. Maybe we can start with that for now and see if people want a more specialize command like porter bundle output list/show to work directly with outputs. We may never need to implement it, depending on lack of asks/feedback.

How does that sound?

That sounds good to me. My only question revolves around the case when an actual output value is large (think: kubeconfig file) while listing. Shall we just default to truncating all values at a certain character limit when printing via default/table and print the entire value if the printing option is otherwise (yaml or json)?

Let's start with truncating at certain number of characters and see how well that works.

Ok, updated to limit cli scope to just the porter bundle show command. Please take a look when convenient.

cmd/porter/outputs.go

carolynvs

This PR will also need to update the base porters schema template to include the outputs section

…er driver used

…ow_test.go

carolynvs

Still working through the PR. Looking good but thought that I'd give some more feedback while I keep reviewing.

carolynvs · 2019-07-10T17:41:50Z

examples/kubernetes-mixin-example/porter.yaml

 uninstall:
  - kubernetes:
      description: "Uninstall Hello World"
+      manifests:


nice catch! 😅

carolynvs · 2019-07-10T17:51:46Z

pkg/cnab/provider/duffle.go

 )

+type Provider interface {


This interface is defined in the package that uses it (not the packge that implements it) which is a bit of a "go ism". So you should remove this interface declaration here, and move FetchClaim to pkg/porter/cnab.go CNABProvider

carolynvs · 2019-07-10T17:55:33Z

pkg/cnab/provider/duffle.go

@@ -34,5 +43,41 @@ func (d *Duffle) newDriver(driverName string) (driver.Driver, error) {
 		configurable.SetConfig(driverCfg)
 	}

+	// If docker driver, setup host bind mount for outputs
+	// TODO: separate function/add tests
+	if dockerish, ok := driverImpl.(*duffledriver.DockerDriver); ok {


It would be nice to see this logic separated into a function (like pkg/cnab/provider/credentials.go loadCredentials).

Separated, though unfortunately we can't yet test as thoroughly as we'd wish (no access yet to the docker driver configuration options... TODO note added.)

carolynvs · 2019-07-10T18:00:02Z

pkg/cnab/provider/duffle.go

+
+		// Create outputs sub-directory using the bundle name
+		bundleOutputsDir := filepath.Join(outputsDir, claimName)
+		err = d.FileSystem.MkdirAll(bundleOutputsDir, 0755)


I think the permissions on that folder may be wrong, because it lets anyone on the machine read outputs created by the bundle. Let's do a sweep on permissions in a separate issue. I'll make one so that we can use a variable for this stuff going forward.

I've changed to os.ModePerm for now... w/ intentions to follow-up across codebase in issue you've mentioned...

carolynvs · 2019-07-10T18:00:59Z

pkg/cnab/provider/duffle.go

 	return driverImpl, err
 }
+
+func (d *Duffle) FetchClaim(name string) (*claim.Claim, error) {


carolynvs · 2019-07-10T18:06:11Z

pkg/cnab/provider/helpers.go

+}
+
+type TestStore struct {
+	backingStore map[string][]byte


I feel bad because you implemented all of this but I'm pretty sure we could have also used afero here and not written a test backing store at all. It would be an afero in memory file system from the test context (which we got from the constructor of the TestDuffle) and then the existing crud claim store.

I think that would be preferable to reimplementing another claim store? Let me know i you'd like to collaborate quick on how to use afero to do that.

Oops! Good call; using afero was the way to go. This file has since been removed.

carolynvs · 2019-07-10T18:07:29Z

pkg/config/manifest.go

+		return errors.New("output name is required")
+	}
+
+	// TODO: Validate inline Schema


Haha, yeah that's a big task on our plate in general for CNAB 1.0 no need to tackle it as part of this PR. 😀

carolynvs · 2019-07-10T18:17:27Z

pkg/porter/build.go

+		return errors.Wrap(err, "unable to get outputs directory")
+	}
+
+	err = p.FileSystem.MkdirAll(outputsDir, 0755)


I'm a bit unsure of the code's intent right here? This function is meant to create files on the local file system in pwd/.cnab in preparation before building the invocation image.

If we need to make the outputs directory ahead of time, then the code should be doing this:

p.FileSystem.MkdirAll(filepath.Join(build.LOCAL_APP, "outputs"), 0755)

Otherwise if your intent was to make sure the outputs directory in PORTER_HOME exists before the bundle is run (which is what this code is doing), then it's in the wrong spot. It should be located in maybe pkg/porter/options.go applyDefaultOptions or really a new function that is executed in each function before calling the cnab provider functions for install/upgrade/etc.

You know, I think we're okay with not ensuring the outputs directory exists before a bundle is run. The logic around setting up the volume mount (if driver is the default Docker driver) is sufficient (will create ${PORTER_HOME}/outputs if it doesn't already exist, in addition to the <claim name> subdir.)

So, I've removed for now.

carolynvs · 2019-07-10T18:19:03Z

pkg/porter/list.go

@@ -40,6 +40,8 @@ func (l CondensedClaimList) Less(i, j int) bool {

 // ListBundles lists installed bundles using the printer.Format provided
 func (p *Porter) ListBundles(opts printer.PrintOptions) error {
+	// TODO: supply cnab.Provider interface as second arg, use to access ClaimStore


Once you move FetchClaim like I suggested above, then you will immediate have access to it, because CNABProvider is already on the Porter receiver here.

vdice · 2019-07-10T20:45:24Z

Ok, latest commits are up intending to address all feedback.

Lemme know if there are any outstanding items; otherwise I/we can squash commits and merge when ready.

…apFs()

carolynvs

This is good to merge. You've got a ton of commits but I wasn't sure if you wanted to do one big squash or an artisanal rebase. Up to you, feel free to merge at will! 👍

vdice force-pushed the feat/bundle-outputs branch 2 times, most recently from de619e6 to 759d16d Compare July 3, 2019 18:11

vdice marked this pull request as ready for review July 8, 2019 16:42

carolynvs reviewed Jul 8, 2019

View reviewed changes

vdice force-pushed the feat/bundle-outputs branch 4 times, most recently from fa0cae3 to 03d18f8 Compare July 10, 2019 00:44

vdice added 12 commits July 10, 2019 10:02

feat(*): add bundle outputs

d5bc461

feat(*): actually write bundle outputs to local dir when default dock…

b3e8d73

…er driver used

update Gopkg.lock; provide bundle name to newDriver

33d5a0d

feat(bundle outputs): add unit and integration/cli test coverage

413714a

feat(adapter_test.go): add generateBundleOutputs test

e4d659b

feat(run.go): create ApplyBundleOutputs function; add unit tests

21b5921

feat(outputs): add outputs-related cli commands

dcf5877

feat(porter/outputs): add ListBundleOutputs unit test

9c99c48

ref(test-cli): update wordpress bundle and test

a68c07d

feat(outputs): add porter bundle show command; misc review updates

dc51d11

add cnab.Provider interface, TestDuffle, TestClaimStore impls; add sh…

5ee56db

…ow_test.go

feat(terraform test bundle): add outputs test/check

53b63cd

carolynvs requested changes Jul 10, 2019

View reviewed changes

feat(outputs): update schema, add documentation

cbe30f3

vdice force-pushed the feat/bundle-outputs branch from 03d18f8 to cbe30f3 Compare July 10, 2019 18:59

vdice added 3 commits July 10, 2019 14:37

address review feedback

f88111d

add/update .gitignore files in testdata bundle dirs

43eb628

docs(authoring-bundles.md): minor amendment

930da8a

fix(helpers.go): TestCNABProvider FileSystem should use afero.NewMemM…

6dcdd75

…apFs()

carolynvs approved these changes Jul 10, 2019

View reviewed changes

vdice merged commit c3908ac into getporter:master Jul 11, 2019

vdice deleted the feat/bundle-outputs branch July 11, 2019 00:38

vdice mentioned this pull request Jul 11, 2019

[Spec]: Support CNAB Bundle Outputs #344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: bundle outputs #438

Feat: bundle outputs #438

vdice commented Jul 1, 2019 •

edited

Loading

vdice commented Jul 3, 2019

vdice commented Jul 8, 2019

carolynvs left a comment

carolynvs Jul 8, 2019

vdice Jul 8, 2019 •

edited

Loading

carolynvs Jul 8, 2019

vdice Jul 8, 2019

carolynvs Jul 9, 2019

vdice Jul 10, 2019

carolynvs left a comment

carolynvs left a comment

carolynvs Jul 10, 2019

carolynvs Jul 10, 2019

carolynvs Jul 10, 2019

vdice Jul 10, 2019

carolynvs Jul 10, 2019

vdice Jul 10, 2019

carolynvs Jul 10, 2019

carolynvs Jul 10, 2019

vdice Jul 10, 2019

carolynvs Jul 10, 2019

carolynvs Jul 10, 2019

vdice Jul 10, 2019

carolynvs Jul 10, 2019

vdice commented Jul 10, 2019

carolynvs left a comment

Feat: bundle outputs #438

Feat: bundle outputs #438

Conversation

vdice commented Jul 1, 2019 • edited Loading

vdice commented Jul 3, 2019

vdice commented Jul 8, 2019

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdice Jul 8, 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

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs 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

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

vdice commented Jul 10, 2019

carolynvs left a comment

Choose a reason for hiding this comment

vdice commented Jul 1, 2019 •

edited

Loading

vdice Jul 8, 2019 •

edited

Loading