Use ConfigArgParse instead of argparse, to support getting parameters from config file and/or env vars. #1167

cyberw · 2019-11-20T13:23:19Z

… from config file and/or env vars.

cyberw · 2019-11-20T13:28:24Z

I like how this change only required me to change 3 lines of code (or really just one line of code, and two lines regarding imports)

❤️ Python

codecov · 2019-11-20T13:34:15Z

Codecov Report

Merging #1167 into master will increase coverage by 0.1%.
The diff coverage is 75%.

@@            Coverage Diff            @@
##           master    #1167     +/-   ##
=========================================
+ Coverage   79.31%   79.41%   +0.1%     
=========================================
  Files          20       20             
  Lines        1895     1895             
  Branches      294      294             
=========================================
+ Hits         1503     1505      +2     
+ Misses        320      319      -1     
+ Partials       72       71      -1

Impacted Files	Coverage Δ
locust/main.py	`34.78% <75%> (ø)`	⬆️
locust/contrib/fasthttp.py	`89.71% <0%> (+1.14%)`	⬆️

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 aa6f5f1...42d1720. Read the comment docs.

…llow clicking it in your terminal.

heyman · 2019-11-20T14:57:23Z

Great!

It'd be nice to have a simple test for the arg parsing, and one that checks that the environment variable settings work. We would probably have to make main.parse_options() take an additional optional args argument which should be passed along to parser.parse_args, in order to achive that.

Also, maybe it's worth adding a short example to the Documentation as well? For someone unfamiliar with ConfigArgParse I imagine that this section can seem a bit dense.

cyberw · 2019-11-20T16:34:45Z

Hmm... I tried adding a test case but ended up breaking parse_options instead. I dont think we need a test, because we're only relying on normal ConfigArgParse functionality.

There is a short example to the docs, do you think I should add more? I dont want to bloat the "quickstart" section. The link goes to the right place (config file syntax explanation) for configargparse on desktop (which should be readable to a programmer), but doesnt work on mobile. Maybe that's what happened?

cyberw · 2019-11-21T08:28:42Z

What I could do (without changing the method signature & breaking stuff) is test reading from config & env vars (but not overriding on command line)

heyman · 2019-11-21T13:37:31Z

I dont think we need a test, because we're only relying on normal ConfigArgParse functionality.

As long as ConfigArgParse doesn't change their API or break in some future version, it should be fine. But I still think it would be nice to have an integration test for that.

I've added a test for parse_options() to master in 009d104. You can merge in master and then make another test for overriding some argument in env variables.

There is a short example to the docs, do you think I should add more?

Ah, nice! For some reason I had missed the environment variable example.

The link goes to the right place (config file syntax explanation) for configargparse on desktop (which should be readable to a programmer)

To me that page feels much more targeted towards users of ConfigArgParse (which makes sense). The first paragraph talks about implementation details of ConfigArgParse. Also it mentions two different file syntaxes for config files. I assume we use the default one, but maybe not clear to Locust end users? Though we could always improve the documentation later.

cyberw · 2019-11-21T13:44:16Z

I dont think we need a test, because we're only relying on normal ConfigArgParse functionality.

As long as ConfigArgParse doesn't change their API or break in some future version, it should be fine. But I still think it would be nice to have an integration test for that.

I've added a test for parse_options() to master in 009d104. You can merge in master and then make another test for overriding some argument in env variables.

Will do! I figured out that the issue I was having was with config files, not with adding an argument to parse: My main config file, in ~/.locust.conf, was interfering with the tests!

I'm looking at a workaround, either by temporarily changing $HOME in the tests (ugly) or detecting that we are running a test in parse_options (also ugly)

The link goes to the right place (config file syntax explanation) for configargparse on desktop (which should be readable to a programmer)

To me that page feels much more targeted towards users of ConfigArgParse (which makes sense). The first paragraph talks about implementation details of ConfigArgParse. Also it mentions two different file syntaxes for config files. I assume we use the default one, but maybe not clear to Locust end users? Though we could always improve the documentation later.

Users can use any of the different syntaxes, ConfigArgParse detects which one you are using.

…parameters-from-conf-file-and-env-vars

heyman · 2019-11-21T13:50:41Z

Maybe you could add a default_config_files argument to parse_options() and use that to specify some temporary file in the test?

cyberw · 2019-11-21T14:03:11Z

Maybe you could add a default_config_files argument to parse_options() and use that to specify some temporary file in the test?

Good idea. I'll try that.

…onfig file & configuration via env vars.

cyberw · 2019-11-21T14:33:52Z

LGTY?

On a slightly unrelated topic, I'd like to make the following change:

... because the tox-installed python annoys MacOS's software firewall (every time I run I get a dialogue asking for permission to open the port). A unit test has no business opening public ports anyway :)

heyman · 2019-11-21T14:54:42Z

locust/test/test_parser.py

@@ -32,3 +33,16 @@ def test_skip_log_setup(self):
        ]
        opts = self.parser.parse_args(args)
        self.assertEqual(opts.skip_log_setup, True)
+
+    def test_parameter_parsing(self):
+        conf_file = '/tmp/locust.conf'


Can we use NamedTemporaryFile here? (https://docs.python.org/2/library/tempfile.html#tempfile.NamedTemporaryFile)

Sure, I'll change that!

heyman · 2019-11-21T14:58:03Z

A unit test has no business opening public ports anyway :)

Agreed. The second change you took a screenshot of should also use 0 as port, instead of hardcode 8888.

…ware firewalls). Piggybacking on this PR.

…t code is run in zmqrpc depending on whether the port is zero (randomize port) or not. Codecov warning was actually correct!

cyberw · 2019-11-22T08:33:08Z

@heyman LGTY?

locust/test/test_zmqrpc.py

heyman · 2019-11-25T15:47:20Z

Looks good!

Use ConfigArgParse instead of argparse, to support getting parameters…

c1e9d38

… from config file and/or env vars.

Tiny doc changes.

7004b50

Drive by fix of help text for no-web. Print web host as real url to a…

178c2fe

…llow clicking it in your terminal.

Merge branch 'master' of https://github.com/locustio/locust into get-…

d2cb9d4

…parameters-from-conf-file-and-env-vars

Parametrize conf file location to facilitate testing,. Add test for c…

a2e7ae5

…onfig file & configuration via env vars.

heyman reviewed Nov 21, 2019

View reviewed changes

cyberw added 3 commits November 21, 2019 16:16

Use tempfile.NamedTemporaryFile instead of a made up file name.

24fda18

Dont open public ports as part of test (mostly because it annoys soft…

49a5825

…ware firewalls). Piggybacking on this PR.

Undo the change where I set the port number to zero, because differen…

42d1720

…t code is run in zmqrpc depending on whether the port is zero (randomize port) or not. Codecov warning was actually correct!

heyman reviewed Nov 25, 2019

View reviewed changes

locust/test/test_zmqrpc.py Show resolved Hide resolved

cyberw merged commit 48d34d3 into locustio:master Nov 25, 2019

cyberw mentioned this pull request Jan 24, 2020

error: argument --master-port: invalid int value bug #1226

Closed

cyberw deleted the get-parameters-from-conf-file-and-env-vars branch January 29, 2020 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ConfigArgParse instead of argparse, to support getting parameters from config file and/or env vars. #1167

Use ConfigArgParse instead of argparse, to support getting parameters from config file and/or env vars. #1167

cyberw commented Nov 20, 2019

cyberw commented Nov 20, 2019

codecov bot commented Nov 20, 2019 •

edited

Loading

heyman commented Nov 20, 2019

cyberw commented Nov 20, 2019 •

edited

Loading

cyberw commented Nov 21, 2019

heyman commented Nov 21, 2019

cyberw commented Nov 21, 2019 •

edited

Loading

heyman commented Nov 21, 2019

cyberw commented Nov 21, 2019

cyberw commented Nov 21, 2019

heyman Nov 21, 2019

cyberw Nov 21, 2019

heyman commented Nov 21, 2019

cyberw commented Nov 22, 2019

heyman commented Nov 25, 2019

Use ConfigArgParse instead of argparse, to support getting parameters from config file and/or env vars. #1167

Use ConfigArgParse instead of argparse, to support getting parameters from config file and/or env vars. #1167

Conversation

cyberw commented Nov 20, 2019

cyberw commented Nov 20, 2019

codecov bot commented Nov 20, 2019 • edited Loading

Codecov Report

heyman commented Nov 20, 2019

cyberw commented Nov 20, 2019 • edited Loading

cyberw commented Nov 21, 2019

heyman commented Nov 21, 2019

cyberw commented Nov 21, 2019 • edited Loading

heyman commented Nov 21, 2019

cyberw commented Nov 21, 2019

cyberw commented Nov 21, 2019

heyman Nov 21, 2019

Choose a reason for hiding this comment

cyberw Nov 21, 2019

Choose a reason for hiding this comment

heyman commented Nov 21, 2019

cyberw commented Nov 22, 2019

heyman commented Nov 25, 2019

codecov bot commented Nov 20, 2019 •

edited

Loading

cyberw commented Nov 20, 2019 •

edited

Loading

cyberw commented Nov 21, 2019 •

edited

Loading