Allow using default params

[divbhasin]

What does this change

Adds a new default value option to the porter parameters generate ... command survey if there is a default value defined for the chosen parameter in the bundle.

To check, we can run porter parameters show ... and see that the default value has been set:

What issue does it fix

Closes #1604

If there is not an existing issue, please make sure we have context on why this change is needed. See our Contributing Guide for examples of when an existing issue isn't necessary.

Notes for the reviewer

Environment variable is still the default option. Not sure if we should the "default value" option the new default for when the survey opens. Also, would appreciate tips on if there are better approaches to organize the code.

Checklist

• Unit Tests
• Documentation
• Schema (porter.yaml)

[carolynvs]

Overall this is going in the right direction! 👍

I want to call out that this is a slightly different set of functionality than what was outlined in #1604. The issue originally asked that when the user selects "use default value" that the parameter would not be included in the parameter set. By not specifying the value, when the parameter set is used, the default value of the bundle is used instead.

This will instead lock in the parameter set to the default value, and if the bundle changes the default value, continue to use the value set.

So you will need to change the implementation to not persist a parameter value when the user selects "use default" instead.

[carolynvs]

If this function returns an error, we should handle it here instead of silently ignoring it.

[divbhasin]

@carolynvs I don't know what would be the best way to handle this error. In most cases, it means there is no default value found, so in my opinion, we should ignore and move on so the user can select from any of the other options. We can perhaps a log a message saying this param does not have a default value according to the bundle. Returning the error here is currently also causing a test to fail.

[carolynvs]

Parameters can default to an empty string, and can also be other types than string. One way to deal with that is to make defaultVal an interface{}, and check for null.

When we need to print the value, this is the logic:

porter/pkg/porter/parameters.go

Lines 385 to 395 in 2b176ec

	var printedValue string
	switch val := v.Value.(type) {
	case string:
	printedValue = val
	default:
	b, err := json.Marshal(v.Value)
	if err != nil {
	return "error rendering value"
	}
	printedValue = string(b)
	}

Which handles when the parameter is a complex type such as an array or object.

[carolynvs]

We can return the default value directly without any comparison or formatting, because we will render the value for display in the genSurvey function.

[divbhasin]

@carolynvs Made the changes that you requested. The first screenshot remains but the second screenshot now changes to:

If I am understanding correctly, we should leave the param value empty like above and it will just get filled in with the default at run time.

[carolynvs]

@divbhasin The expectation is that when I generate parameters and select a default, that parameter is not included in the parameter set at all. So in the second screenshot, you would not need the name parameter listed at all.

For example, this is what it generates now with your PR:

$ ./bin/porter parameters show pr1646 -o json
{
  "schemaVersion": "1.0.0-DRAFT+TODO",
  "name": "pr1646",
  "created": "2021-07-02T10:28:31.385435-05:00",
  "modified": "2021-07-02T10:28:31.385435-05:00",
  "parameters": [
    {
      "name": "name",
      "source": null
    }
  ]
}

and this is what we need:

$ ./bin/porter parameters show pr1646 -o json
{
  "schemaVersion": "1.0.0-DRAFT+TODO",
  "name": "pr1646",
  "created": "2021-07-02T10:28:31.385435-05:00",
  "modified": "2021-07-02T10:28:31.385435-05:00",
  "parameters": []
}

[carolynvs]

@divbhasin Looks like there is a failing unit test related to your change TestGoodParametersName. Let me know if you need help fixing it!

[kichristensen]

A related PR have been merged, #3100, allowing skipping a value instead of showing the default value