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

Parse args from config files independently #395

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

imcdo
Copy link
Member

@imcdo imcdo commented Dec 6, 2023

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:

--exclude c/a_test.py, c/b_test.py

and you run:

ducktape c

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:

--exclude systests/cluster/test_runner_operations.py::SimpleRunnerTest.timeout_test

and executed

ducktape systests/cluster/test_runner_operations.py --max-parallel 9000

which successfully ran our tests, in parallel.

@imcdo imcdo requested a review from a team December 6, 2023 19:13
Copy link

cla-assistant bot commented Dec 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Dec 6, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

stan-is-hate
stan-is-hate previously approved these changes Dec 6, 2023
Copy link
Member

@stan-is-hate stan-is-hate left a 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:
Copy link
Member

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?

Copy link
Member Author

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)

@imcdo imcdo enabled auto-merge (squash) December 6, 2023 23:00
@imcdo imcdo disabled auto-merge December 6, 2023 23:00
@imcdo imcdo merged commit e40783e into confluentinc:master Dec 6, 2023
3 of 4 checks passed
@imcdo imcdo deleted the imcdo/parse-arg-files-individually branch December 6, 2023 23:01
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.

2 participants