cnabio / cnab-go

A Go implementation of CNAB Core 1.0
MIT License
69 stars 35 forks source link

Optional parameters are not injected properly #266

Open carolynvs opened 3 years ago

carolynvs commented 3 years ago

When a parameter is not required, and does not have a default value, the spec says that an empty string should be used.

https://github.com/cnabio/cnab-spec/blob/main/103-bundle-runtime.md#setting-parameter-values

If no value is provided and default is unset, the runtime MUST set the value to an empty string (""), regardless of type.

When we convert to a json representation of the parameter value (which meets another requirement of the same part of the spec), we have special handling so that strings aren't injected as "value". We also need special handling for unspecified parameters though. They are being injected as null.

The other problem is that parameters can also be injected as files. Having an optional file show up as an empty file (assuming the null value is fixed), causes problems for common things, like injecting a tfstate file when available. Injecting an empty file causes most tooling to not work properly because they work on the assumption that if the file is there, it's populated correctly.

I suggest that we also fix the logic to omit the file when it is optional and no value is set (or defaulted). Checking if a file exists is much more of a common pattern than if the file is empty.

vdice commented 2 years ago

I suggest that we also fix the logic to omit the file when it is optional and no value is set (or defaulted). Checking if a file exists is much more of a common pattern than if the file is empty.

Thumbs up to this ^^. Agree that this is preferable to creating an empty file.