--include-system-env-vars doesn't check for the invalid environment variables

[olegbespalov]

Brief summary

Currently, when using the --include-system-env-vars flag, it's possible to also include the environment variables with invalid names like LOREM-IPSUM.

Ideally, the same validation criteria should be applied as those used for the --env. The only exception probably is instead of the erroring, we should print a warning that something like environment variable LOREM-IPSUM is invalid and will be ignored.

k6 version

v0.39.0

OS

linux

Docker version and image (if applicable)

No response

Steps to reproduce the problem

env -i 'LOREM-ISPUM=hello' ./k6 archive --include-system-env-vars script.js

Expected behaviour

LOREM-ISPUM will be excluded since it's invalid

Actual behaviour

It stays in the archive's metadata

[olegbespalov]

Also maybe as part of this issue, we can document grafana/k6-docs that we aren't accept invalid environment variables

[mstoykov]

It seems a bit strange to me to enfornce what "valid" environment variable name is if we got it from the environment.

How exactly will we get such a variable and if we get it why would we argue it's invalid if we got it from the environment. From a quick test that doesn't seem to be possible in my local bash version btw.

And if it is possible k6 run will be spewing warning with the changes from #2652 .

As far as I can see the only benefit from this change is that if somebody has shell that doesn't prevent the usage of invalid names, we will prevent them from using them in the script through __ENV.

But in that case maybe just prevent __ENV from accessing variables with invalid names?

edit: I do wonder if some windows shell or windows in general doesn't have environment variables that are not complient with the POSIX guidelines

[na--]

IIRC we have observed this problem with some weird CI environment that allowed for non-POSIX env vars... Vanilla Windows, Linux and *nix are thankfully saner. I can imagine they have all sorts of weird edge cases in custom CI pipelines...

[mstoykov]

Okay then with the current approach we will potentially be throwing warnings in CI and it is very likely and nobody will even be able to (sanely) write scripts that will use it as it will be impossible to run them anywhere else but in those CIs.

[na--]

Could be :thinking: But it's still a better alternative than incorporating these broken variables in the .tar archive and then being unable to run them in the cloud when someone runs k6 cloud --include-system-env-vars=true script.js with such a broken environment. At least there will be a warning.

Just silently accepting them in the archive and the script's __ENV is not ideal, we should have the same validation for them as we do for --env. And completely aborting the script when there's a wonky environment variable also doesn't seem ideal, since k6 run has --include-system-env-vars enabled by default, and this might make the PR into a big breaking change for users that have such wonky env vars on their systems but don't actually depend on them in their k6 scripts... :thinking:

We will still abort with a configuration error if someone tries to explicitly pass k6 run --env WRONG-VAR=foo or something, since we know that was explicitly meant for k6. But it feels a bit too strict and capricious for k6 to abort based on random junk in the existing environment, which might be a common occurrence. Just ignoring such junk variables with a warning seems like the least bad approach?