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

Optional parameters are not injected properly #266

Open
carolynvs opened this issue Aug 22, 2021 · 1 comment
Open

Optional parameters are not injected properly #266

carolynvs opened this issue Aug 22, 2021 · 1 comment
Labels
bug Something isn't working spec Support new spec changes or fix gaps with the spec

Comments

@carolynvs
Copy link
Contributor

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.

@carolynvs carolynvs added bug Something isn't working spec Support new spec changes or fix gaps with the spec labels Aug 22, 2021
@vdice
Copy link
Member

vdice commented Sep 15, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working spec Support new spec changes or fix gaps with the spec
Projects
None yet
Development

No branches or pull requests

2 participants