Allow non-string, non-complex values in dotenv output format and exec-env
#1914
Conversation
When decrypting to dotenv we try to escape new line in the values without taking into account the possibility that the value could be something different than a string (e.g. an int). This used to cause a panic when using `decrypt --output-format dotenv`. Signed-off-by: billy4479 <giachi.ellero@gmail.com>
This was previously forbidden for some reason, however if the value is not a complex type there should be no issues. Signed-off-by: billy4479 <giachi.ellero@gmail.com>
|
I'm not sure whether it's a good idea to convert everything to strings implicitly. Especially for booleans this can be dangerous, since everyone has their own idea of how booleans should be represented as strings. The INI store automatically converts everything to strings. I guess that if we do this for DotEnv as well, it should use the same functionality. (There's a function @getsops/maintainers any opinion on whether the DotEnv store should automatically convert simple values (floats, bools, integers, timestamps, ...) to strings? |
felixfontein
left a comment
felixfontein
left a comment
There was a problem hiding this comment.
I think I'm OK with merging this if this uses valToString() from stores/ini/store.go. (#1929 fixes the bug I've mentioned in there.) It's probably best to move valToString() to stores/stores.go for that and rename it to ValToString() so it's public.
| value = fmt.Sprintf("%v", item.Value) | ||
| } | ||
|
|
||
| value = strings.ReplaceAll(value, "\n", "\\n") |
There was a problem hiding this comment.
This line is definitely wrong and must not be there.
| value = strings.ReplaceAll(value, "\n", "\\n") |
| } | ||
|
|
||
| value = strings.ReplaceAll(value, "\n", "\\n") |
There was a problem hiding this comment.
The replace only makes sense for strings:
| } | |
| value = strings.ReplaceAll(value, "\n", "\\n") | |
| } else { | |
| value = strings.ReplaceAll(value, "\n", "\\n") | |
| } |
|
Nice! I will have a look again in the next few days, thanks! Also, what is this project convention in this case? Can I rebase on top of your PR or should I wait for it to get merged first? |
I'm not sure we have a convention for that yet. Both is fine for me, or (as a third choice) you could merge my branch into yours. |
2 participants
Fixes #879 and #1167.
As I wrote in this comment the previous behavior seemed intentional so if this is not the desired behavior please let me know, I will make another PR just addressing the panic described in #879 (maybe with a nicer error message?)