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

Allow : colons in data value flags yaml keys #637

Closed
cari-lynn opened this issue Mar 22, 2022 · 3 comments
Closed

Allow : colons in data value flags yaml keys #637

cari-lynn opened this issue Mar 22, 2022 · 3 comments
Labels
enhancement This issue is a feature request

Comments

@cari-lynn
Copy link
Contributor

cari-lynn commented Mar 22, 2022

Describe the problem/challenge you have
It is valid yaml to have a colon in a YAML key and the -f flag allows it:

$ ytt -f a:key: valid
a:key: valid

However, when using data values flags, this is disallowed:

$ ytt --data-value a:key=valid
$ ytt --data-value-yaml a:key=valid

Note: the below : in the environment variable key is invalid in unix, but may be valid in windows

$ export KEYVAL_a:key=valid
$ ytt --data-values-env KEYVAL
$ export KEYVAL_a:key=valid
$ ytt --data-values-env-yaml KEYVAL

all result in an error:

ytt: Error: Extracting data value from KV:
  Expected library ref to start with '@'

Describe the solution you'd like
Do not report this as an error.

a:key: valid

Anything else you would like to add:
YAML separates a key and a value using a colon and a space. So a colon can appear in a key as long as it's not followed by a space.

Considerations

I believe we should allow data values flags to contain : because it is valid yaml and I want to hear if users are running into this. However, it currently serves a purpose to catch syntax errors when incorrectly using a library reference. For example:
We catch this syntax error: if there are two colons in the library reference: ytt --data-value @libraryName::key=value. If we decide to allow : in keys then this becomes valid.

Current workaround

If you are unable to do this:

$ ytt --data-value a:key=valid

Then put the key in a file and load it

$ cat filename.yml
a:key=valid

$ ytt --data-values-file filename.yml

Please share you thoughts on this.

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@cari-lynn cari-lynn added enhancement This issue is a feature request carvel triage This issue has not yet been triaged for relevance labels Mar 22, 2022
@cppforlife
Copy link
Contributor

im personally -1 on supporting :. there are other chars that we do not support (intentionally) such as __ (double underscore) or . (period) since we use them in special ways to figure out key structure. additionally env vars on various OSes do not support various special chars.

it currently serves a purpose to catch syntax errors when incorrectly using a library reference

this check seems to be quite important as well.

@cari-lynn
Copy link
Contributor Author

Intentionally not supporting special chars in the key makes sense in the context of these flags. They all have a flag value of key=value and it's just the key portion of that we are talking about here.

  • --data-values-env
  • --data-values-env-yaml
  • --data-value
  • --data-value-yaml
  • --data-value-file

That just leaves --data-values-file, which does not allow a key in the flag value, so it is not applicable here.

That reasoning makes sense, we currently disallow other 'valid yaml' syntax in this context. Unix disallows : in env vars, and we haven't heard a use case for it, and that there is a workaround if you must use : in a key of a data value flag. These are reasons to choose providing an error message designed to be friendly to user over allowing this value.

@cari-lynn
Copy link
Contributor Author

During testing for : in --data-value-env keys I learned that this currently is allowed. This issue is not meant to change that.

@github-actions github-actions bot removed the carvel triage This issue has not yet been triaged for relevance label Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a feature request
Projects
None yet
Development

No branches or pull requests

2 participants