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

Switch from StringSlice to StringArray #1931

Merged
merged 15 commits into from
Apr 29, 2022
Merged
Show file tree
Hide file tree
Changes from 11 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
3 changes: 2 additions & 1 deletion CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,5 @@ and we will add you. **All** contributors belong here. 💯
* [Mahendra Bishnoi](https://github.com/mahendrabishnoi2)
* [Yingrong Zhao](https://github.com/VinozzZ)
* [Saksham Sharma](https://github.com/sakkshm26)
* [Jeremy Goss](https://github.com/Jemgoss)
* [Jeremy Goss](https://github.com/Jemgoss)
* [Chioma Onyekpere](https://github.com/Simpcyclassy)
8 changes: 4 additions & 4 deletions cmd/porter/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,9 @@ For example, the 'debug' driver may be specified, which simply logs the info giv
"Path to the porter manifest file. Defaults to the bundle in the current directory.")
f.StringVar(&opts.CNABFile, "cnab-file", "",
"Path to the CNAB bundle.json file.")
f.StringSliceVarP(&opts.ParameterSets, "parameter-set", "p", nil,
f.StringArrayVarP(&opts.ParameterSets, "parameter-set", "p", nil,
Simpcyclassy marked this conversation as resolved.
Show resolved Hide resolved
"Name of a parameter set for the bundle. It should be a named set of parameters and may be specified multiple times.")
f.StringSliceVar(&opts.Params, "param", nil,
f.StringArrayVar(&opts.Params, "param", nil,
Simpcyclassy marked this conversation as resolved.
Show resolved Hide resolved
"Define an individual parameter in the form NAME=VALUE. Overrides parameters otherwise set via --parameter-set. May be specified multiple times.")
f.StringSliceVarP(&opts.CredentialIdentifiers, "cred", "c", nil,
"Credential to use when installing the bundle. It should be a named set of credentials and may be specified multiple times.")
Expand Down Expand Up @@ -220,9 +220,9 @@ For example, the 'debug' driver may be specified, which simply logs the info giv
"Path to the porter manifest file. Defaults to the bundle in the current directory.")
f.StringVar(&opts.CNABFile, "cnab-file", "",
"Path to the CNAB bundle.json file.")
f.StringSliceVarP(&opts.ParameterSets, "parameter-set", "p", nil,
f.StringArrayVarP(&opts.ParameterSets, "parameter-set", "p", nil,
"Name of a parameter set file for the bundle. May be either a named set of parameters or a filepath, and specified multiple times.")
f.StringSliceVar(&opts.Params, "param", nil,
f.StringArrayVar(&opts.Params, "param", nil,
"Define an individual parameter in the form NAME=VALUE. Overrides parameters otherwise set via --parameter-set. May be specified multiple times.")
f.StringSliceVarP(&opts.CredentialIdentifiers, "cred", "c", nil,
"Credential to use when installing the bundle. It should be a named set of credentials and may be specified multiple times.")
Expand Down
23 changes: 23 additions & 0 deletions tests/integration/install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,26 @@ func TestInstall_withDockerignore(t *testing.T) {
// We should check this once https://github.com/cnabio/cnab-go/issues/78 is closed
require.EqualError(t, err, "1 error occurred:\n\t* container exit code: 1, message: <nil>\n\n")
}

func TestInstall_stringParam(t *testing.T) {
t.Parallel()

p := porter.NewTestPorter(t)
defer p.Teardown()
p.SetupIntegrationTest()
p.Debug = false

p.AddTestBundleDir("testdata/bundles/bundle-with-string-params", false)

installOpts := porter.NewInstallOptions()
installOpts.Params = []string{"name=Demo Time"}
Copy link

@sleepypioneer sleepypioneer Apr 18, 2022

Choose a reason for hiding this comment

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

@Simpcyclassy try this, the problem I believe is that our test doesn't have quotes around the parameter. Rereading the issue this is expected #1862

As this should be the given command: $porter install -r getporter/hello-llama:v0.1.1 --param name="demo time"

At least for me locally this passes the tests 🎉 and from reading this again as I understand this is the intended behavior. Using single and double quotes catches both this behavior but also when no quotes were passed so that is incorrect as that would catch potentially other flags passed to the command.

Choose a reason for hiding this comment

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

Hmm on further inspection perhaps this is not a good solution. I am running the code locally and it is not passing through "demo time" only "demo" :(

Choose a reason for hiding this comment

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

⚠️ ignore this


err := installOpts.Validate(context.Background(), []string{}, p.Porter)
require.NoError(t, err)

err = p.InstallBundle(context.Background(), installOpts)
require.NoError(t, err)

output := p.TestConfig.TestContext.GetOutput()
require.Contains(t, output, "Hello, Demo Time", "expected action output to contain provided file contents")
}

Choose a reason for hiding this comment

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

@Simpcyclassy We should add a test after this one for checking the second bug 🐞 ie TestInstall_stringParamWithCommas we can essentially use the same bundle and copy most of the test but change the param name to testing 1, 2, 3 instead of demo time.

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/usr/bin/env bash
set -euo pipefail

install() {
echo Hello, $1
}

upgrade() {
echo $1 2.0
}

uninstall() {
echo Goodbye, $1
}

# Call the requested function and pass the arguments as-is
"$@"
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
name: porter-hello
version: 0.1.0
description: "An example Porter configuration"
registry: getporter

mixins:
- exec

parameters:
- name: name
description: Name for our hello bundle
type: string
default: porter-hello

install:
- exec:
description: "Install Hello World"
command: ./helpers.sh
arguments:
- install
- "'{{ bundle.parameters.name }}'"

upgrade:
- exec:
description: "World 2.0"
command: ./helpers.sh
arguments:
- upgrade

uninstall:
- exec:
description: "Uninstall Hello World"
command: ./helpers.sh
arguments:
- uninstall