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

fix(*): update parameter set logic to support file types #1137

Merged
merged 2 commits into from
Jul 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions pkg/porter/cnab.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ func (o *sharedOptions) Validate(args []string, p *Porter) error {
return err
}

err = p.applyDefaultOptions(o)
Copy link
Member

Choose a reason for hiding this comment

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

ooh sneaky, I like it! πŸ‘

if err != nil {
return err
}

err = o.validateParams(p)
if err != nil {
return err
Expand Down Expand Up @@ -234,11 +239,13 @@ func (o *sharedOptions) parseParams() error {

// parseParamSets parses the variable assignments in ParameterSets.
func (o *sharedOptions) parseParamSets(p *Porter) error {
parsed, err := p.loadParameterSets(o.ParameterSets)
if err != nil {
return errors.Wrapf(err, "unable to process provided parameter sets: %v", o.ParameterSets)
if len(o.ParameterSets) > 0 {
parsed, err := p.loadParameterSets(o.ParameterSets)
if err != nil {
return errors.Wrapf(err, "unable to process provided parameter sets: %v", o.ParameterSets)
}
o.parsedParamSets = parsed
}
o.parsedParamSets = parsed
return nil
}

Expand Down
32 changes: 31 additions & 1 deletion pkg/porter/cnab_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,10 @@ func TestParseParamSets_viaPathOrName(t *testing.T) {
},
}

err := opts.parseParamSets(p.Porter)
err := opts.Validate([]string{}, p.Porter)
assert.NoError(t, err)

err = opts.parseParamSets(p.Porter)
assert.NoError(t, err)

wantParams := map[string]string{
Expand All @@ -114,6 +117,33 @@ func TestParseParamSets_viaPathOrName(t *testing.T) {
assert.Equal(t, wantParams, opts.parsedParamSets, "resolved unexpected parameter values")
}

func TestParseParamSets_FileType(t *testing.T) {
p := NewTestPorter(t)

p.TestConfig.TestContext.AddTestFile("testdata/porter-with-file-param.yaml", "porter.yaml")
p.TestConfig.TestContext.AddTestFile("testdata/paramset-with-file-param.json", "/paramset.json")

opts := sharedOptions{
ParameterSets: []string{
"/paramset.json",
},
bundleFileOptions: bundleFileOptions{
File: "porter.yaml",
},
}

err := opts.Validate([]string{}, p.Porter)
assert.NoError(t, err)

err = opts.parseParamSets(p.Porter)
assert.NoError(t, err)

wantParams := map[string]string{
"my-file-param": "/local/path/to/my-file-param",
}
assert.Equal(t, wantParams, opts.parsedParamSets, "resolved unexpected parameter values")
}

func TestCombineParameters(t *testing.T) {
t.Run("no override present, no parameter set present", func(t *testing.T) {
opts := sharedOptions{}
Expand Down
5 changes: 0 additions & 5 deletions pkg/porter/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ func (p *Porter) InstallBundle(opts InstallOptions) error {
return errors.Wrap(err, "unable to pull bundle before installation")
}

err = p.applyDefaultOptions(&opts.sharedOptions)
if err != nil {
return err
}

err = p.ensureLocalBundleIsUpToDate(opts.bundleFileOptions)
if err != nil {
return err
Expand Down
5 changes: 0 additions & 5 deletions pkg/porter/invoke.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@ func (p *Porter) InvokeBundle(opts InvokeOptions) error {
return errors.Wrap(err, "unable to pull bundle before invoking the custom action")
}

err = p.applyDefaultOptions(&opts.sharedOptions)
if err != nil {
return err
}

err = p.ensureLocalBundleIsUpToDate(opts.bundleFileOptions)
if err != nil {
return err
Expand Down
6 changes: 6 additions & 0 deletions pkg/porter/lifecycle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"path/filepath"
"testing"

"get.porter.sh/porter/pkg/config"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -84,6 +85,10 @@ func TestInstallFromTagIgnoresCurrentBundle(t *testing.T) {

func TestBundleLifecycleOpts_ToActionArgs(t *testing.T) {
p := NewTestPorter(t)
cxt := p.TestConfig.TestContext

// Add manifest which is used to parse parameter sets
cxt.AddTestFile("testdata/porter.yaml", config.Name)

deps := &dependencyExecutioner{}

Expand Down Expand Up @@ -118,6 +123,7 @@ func TestBundleLifecycleOpts_ToActionArgs(t *testing.T) {
sharedOptions: sharedOptions{
bundleFileOptions: bundleFileOptions{
RelocationMapping: "relocation-mapping.json",
File: config.Name,
},
Name: "MyClaim",
Params: []string{
Expand Down
4 changes: 3 additions & 1 deletion pkg/porter/options.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package porter

import "get.porter.sh/porter/pkg/manifest"
import (
"get.porter.sh/porter/pkg/manifest"
)

// applyDefaultOptions applies more advanced defaults to the options
// based on values that beyond just what was supplied by the user
Expand Down
17 changes: 17 additions & 0 deletions pkg/porter/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,23 @@ func (p *Porter) loadParameterSets(params []string) (valuesource.Set, error) {
return nil, err
}

// A parameter may correspond to a Porter-specific parameter type of 'file'
// If so, add value (filepath) directly to map and remove from pset
for _, paramDef := range p.Manifest.Parameters {
if paramDef.Type == "file" {
for i, param := range pset.Parameters {
if param.Name == paramDef.Name {
// Pass through value (filepath) directly to resolvedParameters
resolvedParameters[param.Name] = param.Source.Value
// Eliminate this param from pset to prevent its resolution by
Copy link
Member

Choose a reason for hiding this comment

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

What if instead of loading the manifest here and trying to check the parameters before it gets to the cnab-go code, we wrapped the cnab-go host secret store, and extended it in porter to support our custom file type property?

Do you think it would be possible to change our support for the file type from a "hack" (that we shim in wherever we can fit it) to instead something that we override and support in the same struct/place that cnab-go supports the other types?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may be possible, but my initial concern on going this direction was that this file type has classically been exclusive to Porter. As cnab-go is intended to be the Golang reference library for the CNAB Spec, which doesn't mention this type, I didn't think it was a proper fit. However, I agree: it would be much cleaner to move support/override-ability into this library.

Say we do intend to proceed with adding to the cnab-go library. As there is pre-existing code to clean up (not intro'd in this PR), I'd definitely like to work off of a new issue mentioning this broader scope. The question would then be: do we want this fix (or a variant thereof) in the near-term to ensure functionality until the refactor (and corresponding cnab-go PR(s)) are scheduled to be worked on?

Copy link
Member

@carolynvs carolynvs Jul 20, 2020

Choose a reason for hiding this comment

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

Oops, miscommunication here. I was suggesting that we wrap the type defined in cnab-go responsible for resolving the parameter set and then extend it "inside of porter to support the file type". So the file type would continue to be porter only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thank you. I think I have a better understanding now. I'll work with you to outline a strategy in a follow-up refactor in this area.

// the cnab-go library, which doesn't support this parameter type
pset.Parameters[i] = pset.Parameters[len(pset.Parameters)-1]
pset.Parameters = pset.Parameters[:len(pset.Parameters)-1]
}
}
}
}

rc, err := p.Parameters.ResolveAll(pset)
if err != nil {
return nil, err
Expand Down
13 changes: 13 additions & 0 deletions pkg/porter/testdata/paramset-with-file-param.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "mypset",
"created": "1983-04-18T01:02:03.000000004Z",
"modified": "1983-04-18T01:02:03.000000004Z",
"parameters": [
{
"name": "my-file-param",
"source": {
"path": "/local/path/to/my-file-param"
}
}
]
}
27 changes: 27 additions & 0 deletions pkg/porter/testdata/porter-with-file-param.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
name: HELLO_CUSTOM
version: 0.1.0
description: "A bundle with a custom action"
tag: getporter/porter-hello:v0.1.0

parameters:
- name: my-file-param
description: "My file param"
type: file
path: /container/path/to/file

mixins:
- exec

install:
- exec:
description: "cat file"
command: bash
flags:
c: '"cat /container/path/to/file"'

uninstall:
- exec:
description: "cat file"
command: bash
flags:
c: '"cat /container/path/to/file"'
5 changes: 0 additions & 5 deletions pkg/porter/uninstall.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ func (p *Porter) UninstallBundle(opts UninstallOptions) error {
return errors.Wrap(err, "unable to pull bundle before uninstall")
}

err = p.applyDefaultOptions(&opts.sharedOptions)
if err != nil {
return err
}

err = p.ensureLocalBundleIsUpToDate(opts.bundleFileOptions)
if err != nil {
return err
Expand Down
5 changes: 0 additions & 5 deletions pkg/porter/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ func (p *Porter) UpgradeBundle(opts UpgradeOptions) error {
return errors.Wrap(err, "unable to pull bundle before upgrade")
}

err = p.applyDefaultOptions(&opts.sharedOptions)
if err != nil {
return err
}

err = p.ensureLocalBundleIsUpToDate(opts.bundleFileOptions)
if err != nil {
return err
Expand Down
5 changes: 4 additions & 1 deletion tests/install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,11 @@ func TestInstall_fileParam(t *testing.T) {
p.TestConfig.TestContext.AddTestFile(filepath.Join(p.TestDir, "testdata/bundle-with-file-params.yaml"), "porter.yaml")
p.TestConfig.TestContext.AddTestFile(filepath.Join(p.TestDir, "testdata/helpers.sh"), "helpers.sh")
p.TestConfig.TestContext.AddTestFile(filepath.Join(p.TestDir, "testdata/myfile"), "./myfile")
p.TestConfig.TestContext.AddTestFile(filepath.Join(p.TestDir, "testdata/myotherfile"), "./myotherfile")

installOpts := porter.InstallOptions{}
installOpts.Params = []string{"myfile=./myfile"}
installOpts.ParameterSets = []string{filepath.Join(p.TestDir, "testdata/parameter-set-with-file-param.json")}

err := installOpts.Validate([]string{}, p.Porter)
require.NoError(t, err)
Expand All @@ -67,5 +69,6 @@ func TestInstall_fileParam(t *testing.T) {

claim, err := p.Claims.Read(p.Manifest.Name)
require.NoError(t, err, "could not fetch claim")
require.Equal(t, "Hello World!", claim.Outputs["myfile"], "expected output to match the decoded file contents")
require.Equal(t, "Hello World!", claim.Outputs["myfile"], "expected output 'myfile' to match the decoded file contents")
require.Equal(t, "Hello Other World!", claim.Outputs["myotherfile"], "expected output 'myotherfile' to match the decoded file contents")
}
8 changes: 8 additions & 0 deletions tests/testdata/bundle-with-file-params.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ parameters:
- name: myfile
type: file
path: /root/myfile
- name: myotherfile
type: file
path: /root/myotherfile
# This is added to cover bug fix for https://github.com/deislabs/porter/issues/835
# It will be inherently exercised in install_test.go via a default bundle installation
- name: onlyUpgrade
Expand All @@ -22,6 +25,11 @@ outputs:
type: string
applyTo:
- install
- name: myotherfile
type: file
path: /root/myotherfile
applyTo:
- install

install:
- exec:
Expand Down
1 change: 1 addition & 0 deletions tests/testdata/myotherfile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello Other World!
13 changes: 13 additions & 0 deletions tests/testdata/parameter-set-with-file-param.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "mybun",
"created": "2020-06-23T13:31:06.22727-06:00",
"modified": "2020-06-23T13:31:44.692834-06:00",
"parameters": [
{
"name": "myotherfile",
"source": {
"path": "./myotherfile"
}
}
]
}