-
Notifications
You must be signed in to change notification settings - Fork 97
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
Parse args from config files independently #395
Parse args from config files independently #395
Conversation
|
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.
Looks good, thanks Ian! Just one comment out of curiosity, but looks good overall and thanks for adding a test.
@@ -128,6 +128,19 @@ def config_file_to_args_list(config_file): | |||
return list(itertools.chain(*[line.split() for line in config_lines])) | |||
|
|||
|
|||
def parse_non_default_args(parser: argparse.ArgumentParser, defaults: dict, args: list) -> dict: |
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.
why do you need to remove the defaults?
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.
otherwise the merging doesn't work, as the defaults will override the pervious files contributions. (the parser will always include the defaults when parsing the args)
The Problem
Currently if you add any arguments in your config files that take n arguments, such as exclude, and you have no other arguments, these could potentially get merged with your test path arguments.
For example:
.ducktape/config:
and you run:
the arg parse will interpret c as an additional argument to the exclude, so you will now be excluding all tests.
The Solution
Simply parse each set of args independently, then merge them into one combined group of args!
Testing
Unit test added, also tested with our system tests:
Set config to:
and executed
which successfully ran our tests, in parallel.