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

Options will be passed as ENV_variables from command line #681

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

openmohan
Copy link
Contributor

Fixes: 613
The options sent from cli flags will also be saved as environment variables .

So they can be easily accessed as __ENV.XXXX

eg:
Running
./k6 run --vus 1 --iterations 1 --blacklist-ip 192.168.1.2/8,192.178.12.1/16 --duration 1s --summary-trend-stats "avg,p(95)" --system-tags check,group --tag test=p,l=0 --stage 5s:10,5m:20,10s:5 /media/sf_MCMP/Code/src/github.com/loadimpact/k6/test.js

test.js

export default function() {
    console.log("---START---")
    console.log("VUS" + __ENV.vus)
    console.log("duration" + __ENV.duration)
    console.log("stages" + __ENV.stages)
    console.log("throw" + __ENV.throw)
    console.log("systemTags" + __ENV.systemTags)
    console.log("blackListIPS" + __ENV.blacklistIPs)
    console.log("tags" + __ENV.tags)
    console.log("---END---")
}

Output:

INFO[0002] Setup done
INFO[0004] ---START---
INFO[0004] VUS1
INFO[0004] duration1s
INFO[0004] stages[{"duration":"5s","target":10},{"duration":"5m0s","target":20},{"duration":"10s","target":5}]
INFO[0004] throwfalse
INFO[0004] systemTags{"check":true,"group":true}
INFO[0004] blackListIPS["192.0.0.0/8","192.178.0.0/16"]
INFO[0004] tags{"l":"0","test":"p"}
INFO[0004] ---END---
INFO[0004] tear down done

@CLAassistant
Copy link

CLAassistant commented Jun 23, 2018

CLA assistant check
All committers have signed the CLA.

@openmohan openmohan force-pushed the feature/613 branch 3 times, most recently from 5107007 to fe19b51 Compare June 23, 2018 10:24
@codecov-io
Copy link

codecov-io commented Jun 23, 2018

Codecov Report

Merging #681 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
+ Coverage   64.38%   64.38%   +<.01%     
==========================================
  Files         101      101              
  Lines        8274     8275       +1     
==========================================
+ Hits         5327     5328       +1     
  Misses       2598     2598              
  Partials      349      349
Impacted Files Coverage Δ
lib/options.go 84.5% <ø> (ø) ⬆️
js/runner.go 78.92% <100%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77152a3...e794951. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for trying to fix this issue, unfortunately I don't think this approach is the best way to go about it. I think that instead of exposing the final consolidated options as environment variables, we would be better off if we reuse the exported options variable and just update the final values there.

Also, in your code you're just parsing the CLI flags, while options can also be set via a config file, the exported script options and via environment variables. We already have a single struct in the Go code that has the consolidated option values from all of those possible sources and we just have to export it back to the scripts.

@openmohan
Copy link
Contributor Author

Thanks for helping @na-- .
I have doubt here.
I understand the options variable has to be exported back to scripts but how they should be exposed in scripts. ie: how the real values can be read .
is that via module.exports or
any new export key like rt.Set("__OPTIONS", OPTIONS)
or any other better way

@na--
Copy link
Member

na-- commented Jun 25, 2018

I'm not completely sure, because goja (the JS runtime k6 uses) is a bit complex, but I think we can safely reuse/overwrite the original options variable, i.e. something like rt.Set("options", runner.GetOptions()). This seems like a good place to inject back the consolidated options.

@openmohan
Copy link
Contributor Author

@na-- I have updated my commit , Please check it

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better, thanks!

As noted in the inline comment, I'm not sure that the purpose of the js struct tags in lib.Options is, I don't think that they are used by anything.

Also, it would be very useful if you write a unit test that checks this change before we merge it. Something like the tests here that checks if the javascript code would receive the options that were set by r.SetOptions(). For example, you can have setupTimeout: "1s" in the exported in-script settings and overwrite it with 3s from the outside and throw an exception if the script if the time setup timeout is still 1 second or something like that.

Duration types.NullDuration `json:"duration" envconfig:"duration"`
Iterations null.Int `json:"iterations" envconfig:"iterations"`
Stages []Stage `json:"stages" envconfig:"stages"`
VUs null.Int `json:"vus" envconfig:"vus" js:"vus"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the js: struct tags used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here
when a struct is exported in to goja runtime, the property names are converted to snake_cased if "js" tag is missing in the struct
ie: VUs -> v_us , VUsMax -> v_us_max

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, makes total sense.

@na-- na-- requested a review from luizbafilho July 12, 2018 11:22
@na--
Copy link
Member

na-- commented Jul 13, 2018

Thanks for the test and for squashing the code! I'm merging this and later I'll probably expand the test slightly to also verify that the JS code sees the new options as well

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

Successfully merging this pull request may close these issues.

5 participants