Switch from StringSlice to StringArray #1931

Simpcyclassy · 2022-02-23T18:08:01Z

What does this change

This change will allow for parameters containing a space to be parsed in their entirety.

What issue does it fix

Notes for the reviewer

It was observed that the wrong method was being used from the cobra library, which resulted in parameters with space being split. We switched to using the array method and we added a test to confirm this function acts as expected.

Put any questions or notes for the reviewer here.

Checklist

Did you write tests?
Did you write documentation?
Did you change porter.yaml or a storage document record? Update the corresponding schema file.
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

Comment with /azp run test-porter-release if a magefile or build script was modified
Comment with /azp run porter-integration if it's a non-trivial PR

Signed-off-by: Simpcyclassy <[email protected]>

…r into parameter-space-error Signed-off-by: Simpcyclassy <[email protected]>

Simpcyclassy · 2022-03-02T18:26:51Z

workshop/hello-llama/porter.yaml

@@ -17,18 +17,18 @@ install:
      command: ./helpers.sh
      arguments:
        - install
-        - "{{ bundle.parameters.name }}"
+        - "'{{ bundle.parameters.name }}'"


@carolynvs
We investigated this further and concluded it has to do with how the string is escaped in YAML. This probably requires an update to documentation but additionally requires code changes to existing bundles.

For the scope of this issue, would a change of documentation and a code change to the llama bundle be sufficient or do you see an alternative route?
Our proposal is to extend this documentation to include a note regarding how to handle parameters that include a space. We are unsure about the functionality behind the porter parameters generate command and whether an update here is required.
Lastly, what we see as being out of scope is updating all the other places where YAML variables could include space and therefore could face a similar problem.

Would you agree?

Hmm, I wasn't expecting the wrapping quotes to matter here, since we are parsing this and converting it into a an array of arguments in Go, e.g. ["./helpers.sh", "install", "my parameter"]. I would have expected calling a bash script in Go and passing an argument with a space to work, without the quotes. If you can look into that more, that would be great, otherwise I'll try to when I have time.

From our investigation, we see that the strings get passed separately in the bash script, for example when we passed $1, $2, and so on in the /helpers.sh that was as many words as we could get. We also tried $@ which we did not think was very optimal because this would catch all arguments even when not unique to the command. It looks like the values get read in the YAML file in the same manner.
I have had a couple of other engagements of recent and will investigate this further. In the meantime, if you get a chance to look at this and give me more pointers, that will be great. We came across https://github.com/getporter/porter/blob/release/v1/pkg/exec/builder/execute.go#L102, especially the splitCommand(args) line, we will look at this next.

We looked further into the splitCommand(args) function which was added in this commit. This does look like a possible culprit but not something we will like to change. We are potentially missing some context. Is there a chance that you @carolynvs could give us some context? Thanks in advance!

Adding a note here, that I spotted that the escaping in the porter.yaml was being done in these example bundles for flags, still not sure if they are handled differently somehow

@sleepypioneer @Simpcyclassy I just realized that I completely messed up and linked to the StringArray cobra fix on the wrong pull request! I'm so sorry!

Both (parsing the comma and handling whitespace) were things that needed to be fixed, but I messed up and commented on the wrong pull request. I really didn't mean to derail you.

It sounds like you had a fix working for the space problem too? I think you were on the right track with splitCommand. You really are close, I just accidentally threw at you a second (unreleated) bug on this pull request.

You can either fix both on this PR, or just skip the one failing test (tests/integration/install_test.go) and we can merge what you have here (which does fix the comma bug), then fix the parameter space bug in a second pull request. Whatever works easier for you.

Oh good to know, I didn't catch that there was a second bug for commas. We can look again at the splitCommand function, we stopped looking there, after seeing about the cobra method misuse 🙈 Thanks for your patience, we are super excited to be contributing. We only meet once a week so it takes us a little time but as you said we are really close! Thanks for writing and letting us know!!

@carolynvs this is the issue for the comma #1000 I will write there that this PR will add the fix.

I am not too sure how we can best change the behavior, from reading the code for splitCommand, it is working as intended. We either need to wrap our param in extra quotes or we have to skip this processing for parameters but I am concerned that is undoing the reason this was implemented in the first place?

Adding a little extra context.

If we leave the splitCommand(args) in this is the output we see for first a param with quotes around it and secondly a param without quotes:

greenmachine@pop-os:/tmp/hello-world$ porter install --param name="demo time" --force executing install action from porter-hello (installation: /porter-hello) Install Hello World Hello demo execution completed successfully! greenmachine@pop-os:/tmp/hello-world$ porter install --param name=demo time --force executing install action from porter-hello (installation: /time) Install Hello World Hello demo execution completed successfully!

Now the same with the call to splitCommand removed:

greenmachine@pop-os:/tmp/hello-world$ porter install --param name="demo time" --force executing install action from porter-hello (installation: /porter-hello) Install Hello World Hello demo time execution completed successfully! greenmachine@pop-os:/tmp/hello-world$ porter install --param name=demo time --force executing install action from porter-hello (installation: /time) Install Hello World Hello demo execution completed successfully!

We see that removing the splitCommand call makes things behave as expected, but I am still unsure if there is an edge case we miss that makes it unwise to remove this. We could move the call up and run it only for flags (I haven't dug into how flags use this or are expected to behave) It seems at least to me that what this command was trying to solve is no longer an issue for params.

I am also thinking that in this case arguments != parameters, so we may want to only have an effect on parameters that are passed through as other arguments may still require this behavior from splitCommand looking to see where we could do that.

Signed-off-by: Simpcyclassy <[email protected]>

cmd/porter/bundle.go

sleepypioneer · 2022-04-18T07:28:09Z

@Simpcyclassy our test is failing here with the same error we saw on my computer:

Invalid schema version

You can see it here: https://dev.azure.com/getporter/porter/_build/results?buildId=3568&view=logs&j=c892508d-390a-5765-0ad0-b341c7fd0dd4&t=99b6391b-efca-5f28-e88f-b44afecfc894&l=10900

See my comment below regarding this 👇

cmd/porter/bundle.go

sleepypioneer · 2022-04-18T07:39:29Z

Did you change porter.yaml or a storage document record? Update the corresponding schema file.

So I got curious after reading the checklist in the PR Did you change porter.yaml or a storage document record? Update the corresponding schema file.

I therefore checked the difference between our tests porter.yaml and another tests porter.yaml If you add the schemaVersion this will pass the tests :) Seems this was added after we first started working on this #1946

Signed-off-by: Simpcyclassy <[email protected]>

sleepypioneer · 2022-04-18T15:50:04Z

tests/integration/install_test.go

+	p.AddTestBundleDir("testdata/bundles/bundle-with-string-params", false)
+
+	installOpts := porter.NewInstallOptions()
+	installOpts.Params = []string{"name=Demo Time"}


~~@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.

sleepypioneer · 2022-04-18T16:59:25Z

tests/integration/install_test.go

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


@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.

* Run mage build to generate the cli command documentation to include the recent changes * Skip the failing test that was added. This PR has enough to fix the comma parsing, and a follow-up will fix this failing test for when a parameter has a space in it. Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs

Thank you for the fix! Someone needed the fix on our stable (main) branch, so I'm helping to get it merged today and backported to v0.38).

I have added a commit to your PR so that I can merge what we have working now (The fix for StringSliceVar).

Ran mage build so that the generated cli documenation was updated with your fix
Skipped the failing test case for parameters with spaces. We can uncomment it when fixed.

Signed-off-by: Carolyn Van Slyck <[email protected]>

* test for parameters with space Signed-off-by: Simpcyclassy <[email protected]> * add contributor Signed-off-by: Simpcyclassy <[email protected]> * handle bundle parameter escape Signed-off-by: Simpcyclassy <[email protected]> * update parameters documentation Signed-off-by: Simpcyclassy <[email protected]> * Merge branch 'release/v1' into parameter-space-error Signed-off-by: Simpcyclassy <[email protected]> * array to slice paramenter Signed-off-by: Simpcyclassy <[email protected]> * define params as array var Signed-off-by: Simpcyclassy <[email protected]> * add schema version to yaml file Signed-off-by: Simpcyclassy <[email protected]> * Help get PR merged * Run mage build to generate the cli command documentation to include the recent changes * Skip the failing test that was added. This PR has enough to fix the comma parsing, and a follow-up will fix this failing test for when a parameter has a space in it. Signed-off-by: Carolyn Van Slyck <[email protected]> * Fix merge conflicts with release/v1 Signed-off-by: Carolyn Van Slyck <[email protected]> Co-authored-by: Carolyn Van Slyck <[email protected]> Signed-off-by: Chioma Onyekpere <[email protected]>

Backport Switch from StringSlice to StringArray (#1931)

sleepypioneer · 2022-05-08T19:22:49Z

Congrats @Simpcyclassy on your first contribution 🎉 👏👏👏

Thank you @carolynvs for helping get this merged 💖 we had to take a small break due to personal commitments but we are back now and starting again to fix the parameter with a space issue 😄

Simpcyclassy added 2 commits February 23, 2022 19:01

test for parameters with space

8960743

Signed-off-by: Simpcyclassy <[email protected]>

add contributor

6d1ee05

Signed-off-by: Simpcyclassy <[email protected]>

carolynvs and others added 3 commits February 25, 2022 09:28

Merge branch 'release/v1' into parameter-space-error

3b7c06f

handle bundle parameter escape

fc337aa

Signed-off-by: Simpcyclassy <[email protected]>

Merge branch 'parameter-space-error' of github.com:Simpcyclassy/porte…

2d14f43

…r into parameter-space-error Signed-off-by: Simpcyclassy <[email protected]>

Simpcyclassy commented Mar 2, 2022

View reviewed changes

update parameters documentation

132d348

Signed-off-by: Simpcyclassy <[email protected]>

Simpcyclassy force-pushed the parameter-space-error branch 2 times, most recently from 5b2c955 to 132d348 Compare March 27, 2022 17:28

Simpcyclassy added 5 commits March 27, 2022 18:31

Merge branch 'release/v1' into parameter-space-error

4afb3e9

Signed-off-by: Simpcyclassy <[email protected]>

Merge branch 'release/v1' into parameter-space-error

ac2c6a7

Signed-off-by: Simpcyclassy <[email protected]>

Merge branch 'release/v1' into parameter-space-error

a0e6565

Signed-off-by: Simpcyclassy <[email protected]>

array to slice paramenter

cbce5a4

Signed-off-by: Simpcyclassy <[email protected]>

define params as array var

5ca9a75

Signed-off-by: Simpcyclassy <[email protected]>

sleepypioneer reviewed Apr 18, 2022

View reviewed changes

cmd/porter/bundle.go Show resolved Hide resolved

sleepypioneer reviewed Apr 18, 2022

View reviewed changes

cmd/porter/bundle.go Outdated Show resolved Hide resolved

add schema version to yaml file

dda5218

Signed-off-by: Simpcyclassy <[email protected]>

sleepypioneer reviewed Apr 18, 2022

View reviewed changes

sleepypioneer mentioned this pull request Apr 18, 2022

Param parsing when commas are involved #1000

Closed

sleepypioneer reviewed Apr 18, 2022

View reviewed changes

carolynvs mentioned this pull request Apr 21, 2022

Enabling JSON object as parameter #1917

Closed

4 tasks

joshuabezaleel mentioned this pull request Apr 28, 2022

Enable setting custom values during porter build. #1900

Merged

4 tasks

carolynvs approved these changes Apr 29, 2022

View reviewed changes

carolynvs added 2 commits April 29, 2022 12:27

Merge branch 'release/v1' into pr1931

4b3ecbb

Signed-off-by: Carolyn Van Slyck <[email protected]>

Fix merge conflicts with release/v1

9b1172f

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs marked this pull request as ready for review April 29, 2022 17:44

carolynvs requested review from jeremyrickard and vdice as code owners April 29, 2022 17:44

carolynvs merged commit cba074a into getporter:release/v1 Apr 29, 2022

github-actions bot mentioned this pull request Apr 29, 2022

Parameter with space isn't treated properly #1862

Closed

carolynvs changed the title ~~test for parameters with space~~ Switch from StringSlice to StringArray Apr 29, 2022

carolynvs mentioned this pull request Apr 29, 2022

Backport Switch from StringSlice to StringArray (#1931) #2046

Merged

carolynvs added a commit that referenced this pull request May 2, 2022

Merge pull request #2046 from carolynvs/backport-stringarray-fix

678aaf6

Backport Switch from StringSlice to StringArray (#1931)

carolynvs mentioned this pull request May 25, 2022

Sync from main #2101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from StringSlice to StringArray #1931

Switch from StringSlice to StringArray #1931

Simpcyclassy commented Feb 23, 2022 •

edited by carolynvs

Loading

Simpcyclassy Mar 2, 2022

carolynvs Mar 2, 2022

Simpcyclassy Mar 13, 2022

Simpcyclassy Mar 27, 2022 •

edited

Loading

sleepypioneer Mar 27, 2022

carolynvs Apr 18, 2022

sleepypioneer Apr 18, 2022

sleepypioneer Apr 18, 2022

sleepypioneer Apr 18, 2022

sleepypioneer Apr 18, 2022

sleepypioneer commented Apr 18, 2022 •

edited

Loading

sleepypioneer commented Apr 18, 2022

sleepypioneer Apr 18, 2022 •

edited

Loading

sleepypioneer Apr 18, 2022

carolynvs left a comment

sleepypioneer commented May 8, 2022

Switch from StringSlice to StringArray #1931

Switch from StringSlice to StringArray #1931

Conversation

Simpcyclassy commented Feb 23, 2022 • edited by carolynvs Loading

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simpcyclassy Mar 27, 2022 • 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

sleepypioneer commented Apr 18, 2022 • edited Loading

sleepypioneer commented Apr 18, 2022

sleepypioneer Apr 18, 2022 • edited Loading

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

sleepypioneer commented May 8, 2022

Simpcyclassy commented Feb 23, 2022 •

edited by carolynvs

Loading

Simpcyclassy Mar 27, 2022 •

edited

Loading

sleepypioneer commented Apr 18, 2022 •

edited

Loading

sleepypioneer Apr 18, 2022 •

edited

Loading