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 null values in environment variables #109

Closed
MisinformedDNA opened this issue Apr 1, 2021 · 3 comments · Fixed by #110
Closed

Allow null values in environment variables #109

MisinformedDNA opened this issue Apr 1, 2021 · 3 comments · Fixed by #110

Comments

@MisinformedDNA
Copy link

We are seeing nullability warnings since Command.EnvironmentVariables is of type IReadOnlyDictionary<string, string> and not IReadOnlyDictionary<string, string?>.

I would expect it to be the latter since it maintains consistency with CLR types:

Environment.SetEnvironmentVariable accepts a nullable string as its value and Environment.GetEnvironmentVariable returns a nullable string.

Are you interested in a PR to fix this?

@t0yv0
Copy link

t0yv0 commented Apr 1, 2021

Specifically this may unlock the ability to unset an environment variable via CliWrap which is not currently possible. Consider this test program:

cat debug-boom.sh

#!/bin/sh
var=${BOOM-NA}
echo "BOOM IS $var"

The program can distinguish between empty, unset and set env var BOOM. Here is a session demonstrating that env UNIX command can manipulate this environment.

$ env -u BOOM debug-boom.sh
BOOM IS NA

$ env BOOM="" debug-boom.sh
BOOM IS 

$ env BOOM=1 debug-boom.sh
BOOM IS 1

Note that env can also remove an environment variable even if it is set globally:

$ BOOM=1 env -u BOOM debug-boom.sh
BOOM IS NA

As far as I can tell this functionality is currently missing from CliWrap API. The user cannot remove the environment variable that is set globally. At best the user can set it to an empty value, which is not the same as removing it.

If we allowed the Environment to have null string values, we could potentially interpret nulls as instructions to remove environment variables set globally.

Note that https://docs.microsoft.com/en-us/dotnet/api/system.environment.setenvironmentvariable?view=net-5.0 currently treats empty values and nulls identically, and it treats them as instructions to remove. This is different from current behavior of CliWrap that treats empty values as explicit as-is values and passes them down.

@vipentti
Copy link
Contributor

vipentti commented Apr 1, 2021

I think this actually requires dotnet/runtime#34446 to be fixed

@Tyrrrz Tyrrrz changed the title EnvironmentVariables should allow nulls Allow null values in environment variables Apr 1, 2021
@Tyrrrz
Copy link
Owner

Tyrrrz commented Apr 1, 2021

Sounds good to me. You can send a PR.
Let's treat null values as instruction to remove. Keep empty strings as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants