cli: add filtering support for --list_* options#1367
cli: add filtering support for --list_* options#1367jmartin-tech merged 14 commits intoNVIDIA:mainfrom
Conversation
|
DCO Assistant Lite bot All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO Document and I hereby sign the DCO |
|
recheck |
4f85798 to
74187d5
Compare
leondz
left a comment
There was a problem hiding this comment.
Thanks for this. Would prefer to retain print_plugins functionality for probes & detectors. Can we factor this filtering up into that function, perhaps also covering other plugin types?
jmartin-tech
left a comment
There was a problem hiding this comment.
This is not the layer I would expect to find responsible for this work. All _config and cli argument parsing is preformed by cli.py and loaded into the runtime config state.
In fact all the logic required to determine what plugins will be activated by the config exists in cli.py where it calls _config.parse_plugin_spec().
A refactor of print_plugins() could possibly be a place to receive the filtered set. Depending on the how implemented I could see this refactoring signatures for other print_*() methods to improve code reuse.
|
Hey there, just a heads-up, I accidentally clicked re-request review 😅. Still working on the changes, I’ll push updates soon and ping again when it’s ready! |
Co-authored-by: Leon Derczynski <leonderczynski@gmail.com> Signed-off-by: Joseph Davis Chamdani <joseph.chamdani@gmail.com>
- Remove duplicate imports (typing.Iterable, List and sys) - Remove unused import (typing) - Organize imports above constants - Address reviewer feedback from PR NVIDIA#1362
|
Just finished making all the requested fixes. Hope everything is good now! Let me know if there’s anything else I should tweak. Thanks for the feedback! |
jmartin-tech
left a comment
There was a problem hiding this comment.
The current revision still attempts to evaluate cli arguments in a location that should not be responsible for accessing them. See comments for further detail.
…y, and apply code formatting - Move all CLI/config parsing and plugin selection logic to cli.py, keeping command.py focused on display logic only. - Update print_plugins and related function signatures to use selected_plugins.
|
I just pushed the changes and moved the CLI parsing into cli.py and updated command.py as suggested. Thanks so much for all the feedback and patience throughout this process. This is actually my first open-source PR, so I really appreciate the guidance and the time you’ve taken to help me improve it! 🙏 |
jmartin-tech
left a comment
There was a problem hiding this comment.
Testing shows this is really close to the desired function. I noticed there was some confusion about the desired output. I believe the comment original mentioning family inclusion was based on ensuring the changes did not impact the default display where all values that can validly be offered in a spec are returned with emoji entires to designate family or inactive plugins.
Co-authored-by: Jeffrey Martin <jmartin@Op3n4M3.dev> Signed-off-by: Joseph Davis Chamdani <joseph.chamdani@gmail.com>
|
Thanks for the suggestion! I've committed your recommended change as-is, and the code now matches your review. |
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
|
Hi @jmartin-tech, I saw you made some changes to the code. Is there anything you need me to update or fix now, or should I just wait for review/merge? |
|
@JosephDavisC just wait for now this will either land or a direct ask will be posted. The acceptance process is still ongoing. |
Co-authored-by: Leon Derczynski <leonderczynski@gmail.com> Signed-off-by: Joseph Davis Chamdani <joseph.chamdani@gmail.com>
a6681ff to
ef17758
Compare
Signed-off-by: Joseph Davis Chamdani <joseph.chamdani@gmail.com>
|
Hi there, I’ve added the CLI docs for filtering, so you should see the usage examples in the help output for --list_probes and --list_detectors. Sorry for all the commits, I accidentally pushed some extra files earlier instead of just command.py and cli.py. It should be sorted now! Let me know if anything else needs fixing. Thanks! |
jmartin-tech
left a comment
There was a problem hiding this comment.
The help comments really only make sense when a --list options is passed, some ideas offered that I think are more clear about the impacts of usage and to focus the examples on when they are expected to have an impact on output.
Co-authored-by: Jeffrey Martin <jmartin@Op3n4M3.dev> Signed-off-by: Joseph Davis Chamdani <joseph.chamdani@gmail.com>
Co-authored-by: Jeffrey Martin <jmartin@Op3n4M3.dev> Signed-off-by: Joseph Davis Chamdani <joseph.chamdani@gmail.com>
…etectors help texts Signed-off-by: Joseph Davis Chamdani <joseph.chamdani@gmail.com>
|
Reverted the help text change for --probes as suggested. Thanks for the feedback! Let me know if anything else needs fixing |
Title
cli: add filtering support for --list_probes and --list_detectors (fix #1362)
Description
This PR adds filtering support to the
--list_probesand--list_detectorsCLI options.dan) or by a specific plugin (e.g.,dan.AntiDAN).This resolves issue #1362.
Verification
Steps to verify:
pytest -q tests/test_cli_list_filtering.py(all pass).