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

Enabling JSON object as parameter #1917

Conversation

joshuabezaleel
Copy link
Contributor

What does this change

Consuming JSON object from a file as parameter with the type of "object".

What issue does it fix

Closes #1508

Notes for the reviewer

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][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

@joshuabezaleel
Copy link
Contributor Author

This PR is a work in progress thus marked as a draft. Pushed it as a PR so reviewers can take a look at the WIP code.

porter-1508-consume-parameter-jsonobject.mp4

Hi @carolynvs , I am sorry in advance for pushing a not-finished PR since I guess there are some things that I missed on how things work here.

I am not sure why does the contents of the file from the root folder (which should be {"KEY": "VALUE"}) got replaced by the default value ({"color": "red"}) inside the /cnab/app directory when it was referenced as one of the parameter?

In the video, I tried to print all the contents of the file inside the /cnab/app directory.
From the first example we can see that if we define it as different file name, other-config.json in the root directory and config.json on the parameter, the other-config.json still has the value of {"KEY": "VALUE"}`` and config.json has the value of `{"color": "red"}`, which was expected since we didn't define the content of the `config.json` in advance.
But when we define the `config.json` in the root directory and also referenced it on the parameters, its content get replaced by the default value.
And when I didn't define the default value on the third example, it yields an error saying that the parameter is required.

Thank you lots in advance! 😄

@carolynvs
Copy link
Member

@joshuabezaleel Sorry I'm having trouble following what the problem is based on your description and the video. When a parameter is not specified, the parameter's value is set to the default value defined in the bundle, which based on your video is exactly what is happening.

What did you expect to happen?

@joshuabezaleel
Copy link
Contributor Author

I tried to take a step back and just saw that there's a section for Object Parameters on the Create a Bundle task page on the v1.0.0 docs which didn't exist on the previous version of the docs site. I have tried to follow it since I haven't had any experience with object type parameters but found trouble by using the whalesay example.
At first I thought it was because the command example lack the name of the parameter as key (e.g. should be porter install --param config='{"logLevel":2}') but it still fails anyway. Is there anything wrong with the steps that I've taken?
image
image

@carolynvs
Copy link
Member

carolynvs commented Apr 21, 2022

Here's how to set a parameter value with quotes in it (wrap the entire flag value in the single quotes)

porter install --param 'name={"foo":"bar"}'

That still won't work though because there is a bug in how we are defining the --param flag, which is being fixed in #1931. To work around it change the following line to use StringArrayVar

f.StringSliceVar(&opts.Params, "param", nil,

@joshuabezaleel
Copy link
Contributor Author

@carolynvs Ah I see, while waiting for #1931 to be merged, will it be okay if I close and drop this PR and issue for a while as not to leave this PR dangling and I may be able to work on other issue?

@carolynvs
Copy link
Member

Yeah don't worry about it. Feel free to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants