-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add CLI flags to set environment variables #495
Add CLI flags to set environment variables #495
Conversation
This seems to be the problem with the previous CircleCI run: alecthomas/gometalinter#149
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@na-- I've done a first pass now. Looking good. Commented on some things that needs to be changed/implemented.
Thanks a lot for refactoring some test cases as well 👍
cmd/inspect.go
Outdated
@@ -63,7 +63,8 @@ var inspectCmd = &cobra.Command{ | |||
} | |||
opts = arc.Options | |||
case typeJS: | |||
b, err := js.NewBundle(src, fs) | |||
//TODO: also accept CLI env vars? they can influence the JS init and options | |||
b, err := js.NewBundle(src, fs, lib.RuntimeOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Yes, I think we should also accept -e
env vars for inspect
to set up a proper execution environment as it could influence JS init phase and thus the exported options as you say in the comment.
cmd/runtime_options.go
Outdated
opts.Env = collectEnv() | ||
} | ||
|
||
// Set/overwrite environment varialbes with custom user-supplied values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo "varialbes" -> "variables"
js/bundle.go
Outdated
@@ -158,7 +144,7 @@ func NewBundleFromArchive(arc *lib.Archive) (*Bundle, error) { | |||
Program: pgm, | |||
Options: arc.Options, | |||
BaseInitContext: initctx, | |||
Env: collectEnv(), | |||
Env: arc.Env, //TODO: if set, overwrite some options via rtOpts.Env? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if -e
env vars are specified when running an archive they should override whatever was saved with the archive.
@@ -169,6 +155,7 @@ func (b *Bundle) MakeArchive() *lib.Archive { | |||
Filename: b.Filename, | |||
Data: []byte(b.Source), | |||
Pwd: b.BaseInitContext.pwd, | |||
Env: b.Env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this now, we should never include the env vars from the system in an archive as that will sooner or later cause keys, tokens, passwords etc. to be leaked with an archive that gets passed around, sent to the cloud service etc. So, when creating an archive (as a result of k6 archive ...
or k6 cloud ...
), only include -e
specified env vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds goods, that alleviates my concerns about potential privacy issues with running k6 archive
and k6 cloud
. The only issue remaining then would be the different default behavior between run
and the other commands. I'm assuming that inspect
should behave like archive
and cloud
and use only explicitly passed variables with --env
/-e
while ignoring the real system env by default.
To minimize the impact from the different defaults, I'm considering flipping the --no-system-env-vars
to be called --use-system-env-vars
with default true for run
and false for cloud
/archive
/inspect
. What do you think? It seems to me that would be easier to reason about than the double negatives. Right now with --no-system-env-vars
it would be "by default don't not include system env vars when running tests locally and do not include them when running in the cloud".
I'm planning to implement the fixes this evening or tomorrow, I'll ping you when they're ready for a new review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, flipping --no-system-env-vars
to --use-system-env-vars
(with a default true
for run
command) makes sense. Given that the flag will now need to take a boolean (to support turning off for run
/inspect
and turning on for cloud
/archive
) maybe rename it to just --system-env-vars
(or --include-system-env-vars
)? Are we going to accept "true"|"false" and/or 0|1 as values?
I do think we should include system env vars by default for inspect
though since it's run locally like run
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how the pflag library deals with boolean/set values, I'll check and test it later. Since we'll have different default values for different commands, I'll make sure that it's possible to explicitly specify true
/false
( and/or 0
/1
, enabled/disabled
, etc.). I assume that it should be able to accept values, if not, I can just use a string flag and parse it. I'll also add both cases to the end-to-end tests to make sure everything's OK.
Regarding the name, I think --system-env-vars
would be best if the expected values are enabled
/disabled
, but --use-system-env-vars
/--include-system-env-vars
sound better for true
/false
values.
For the inspect
command - yeah, it's run locally, but I assume that the most common use case is inspecting tests and archive
s which may be executed on your cloud
or on a private CI/CD system somewhere. If that's the case, I don't think that evaluating the user's system environment variables by default is the best strategy when inspect
ing. That said, I'm far from knowledgeable about all of the k6 use cases, so if you think inspect
should behave like run
with regards to environment variables, that's what I'll do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, agree on the naming of the flag. Use the one that fits with the expected value.
You're right, didn't think of the archive case for inspect
. Ok, default to excluding system env vars here.
The default value is true for the run command and false for archive and cloud.
Codecov Report
@@ Coverage Diff @@
## master #495 +/- ##
==========================================
+ Coverage 62.37% 62.52% +0.14%
==========================================
Files 93 95 +2
Lines 6732 6788 +56
==========================================
+ Hits 4199 4244 +45
- Misses 2284 2292 +8
- Partials 249 252 +3
Continue to review full report at Codecov.
|
@robingustafsson, I think everything in the code should now be resolved and ready for a final review. I'll fix the documentation once I have the 👍 for the pull request, just in case there are more changes needed. Just a couple remarks about the latest changes:
|
@na-- Great, I'll have a look. Nice with pflags and yes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@na-- LGTM, just a minor TODO missing. I've tested the code and it works as I'd expect so good job!
lib/archive_test.go
Outdated
@@ -128,3 +128,5 @@ func TestArchiveJSONEscape(t *testing.T) { | |||
assert.NoError(t, err) | |||
assert.Contains(t, string(b), "test<.js") | |||
} | |||
|
|||
//TODO: write test for environment variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, looks like I missed a spot 😄. I think that I already wrote the archive
test I intended to with this TODO as a part of the end-to-end test here, so I'm not sure what exactly I can test that's not already covered by it. Any ideas or should I just delete the TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True that. Ok, just remove this TODO and we'll be ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@na-- Thanks! Merging. |
@na-- Sorry, did a first pass of updating the docs (https://docs.k6.io/v1.0/docs/environment-variables) before I remembered you said you were going to do it. Please feel free to do modifications to that doc and change the small addition I did :) |
This closes #467. I wrote a comment in the issue with some information from a discussion with @robingustafsson about the implementation details, since my initial attempts at writing this hit a bit of a snag.
This should be mostly finished now, it should be mostly ready for code review. The documentation is not yet done, though I think that's outside of the git repo. There are also a few remaining things that I'm not sure how to address:
inspect
command also accept CLI environment variables? I'm leaning towards "yes", since with the old code system environment variables could affect the result ofinspect
metadata.json
by default when runningk6 archive
/k6 cloud
. This may lead to users inadvertently exposing private data. At the same time, disabling the capture of environment variables by default could lead to backwards incomparability withk6 run
(I think) or to inconsistent behaviour between therun
andarchive
/cloud
commands...Please tell me if there's anything I'm missing or haven't considered, I'd be happy to fix it.