traffic_ctl: Add --append option to append debug tags instead of replacing them. (inc ArgParser support).#12804
Conversation
…le usages - Add with_required() method to specify that an option requires another - Change add_example_usage() to support multiple examples per command - Add unit tests for option dependencies - Update ArgParser documentation
… tags - Add --append/-a option to append debug tags instead of replacing them - Uses ArgParser's with_required() to enforce --tags dependency - Add autest for server debug enable/disable commands - Update traffic_ctl documentation
brbzull0
added
Tools
Cleanup
Documentation
traffic_ctl
brbzull0
changed the title
--append option to append debug tags instead of replacing them. (inc ArgParser support).
There was a problem hiding this comment.
Pull request overview
This PR adds a new --append option to the traffic_ctl server debug enable command that allows appending debug tags to existing tags instead of replacing them. It also enhances the ArgParser library with option dependency support via a new with_required() method and updates add_example_usage() to support multiple examples per command.
Changes:
- Added
with_required()method to ArgParser for specifying option dependencies - Modified
add_example_usage()to support multiple example usage strings - Implemented
--append/-aflag intraffic_ctl server debug enablecommand with proper dependency on--tags
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| include/tscore/ArgParser.h | Added with_required() method declaration, changed _example_usage to _example_usages vector, added dependency tracking fields |
| src/tscore/ArgParser.cc | Implemented with_required() and validate_dependencies() methods, updated example usage handling, added dependency info to help output |
| src/tscore/unit_tests/test_ArgParser_OptionDependencies.cc | Comprehensive unit tests for option dependency feature |
| src/tscore/CMakeLists.txt | Added new unit test file to build |
| src/traffic_ctl/traffic_ctl.cc | Added --append option with with_required("--tags") and multiple example usages |
| src/traffic_ctl/CtrlCommands.h | Added APPEND_STR constant |
| src/traffic_ctl/CtrlCommands.cc | Implemented tag appending logic in server_debug() |
| tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py | Added Debug class for testing debug commands |
| tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py | New autest for debug enable/disable with append functionality |
| tests/gold_tests/traffic_ctl/gold/test_2.gold | Removed (no longer needed) |
| tests/gold_tests/traffic_ctl/gold/test_3.gold | Removed (no longer needed) |
| doc/developer-guide/internal-libraries/ArgParser.en.rst | Documented option dependencies feature |
| doc/appendices/command-line/traffic_ctl.en.rst | Documented --append option with examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If append mode is enabled and tags are provided, fetch current tags and combine | ||
| if (append && !tags.empty()) { | ||
| shared::rpc::RecordLookupRequest lookup_request; | ||
| lookup_request.emplace_rec("proxy.config.diags.debug.tags", shared::rpc::NOT_REGEX, shared::rpc::CONFIG_REC_TYPES); | ||
| auto lookup_response = invoke_rpc(lookup_request); | ||
|
|
||
| if (!lookup_response.is_error()) { | ||
| auto const &records = lookup_response.result.as<shared::rpc::RecordLookUpResponse>(); | ||
| if (!records.recordList.empty()) { | ||
| std::string current_tags = records.recordList[0].currentValue; | ||
| if (!current_tags.empty()) { | ||
| // Combine: current|new | ||
| tags = current_tags + "|" + tags; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The append logic doesn't check for duplicate tags. If a user appends a tag that already exists in the current tags, it will be duplicated (e.g., "http|dns|http"). Consider adding deduplication logic to prevent duplicate tags in the combined string. This could be done by splitting the current and new tags, combining them into a set to remove duplicates, and then joining them back together.
serrislew
left a comment
serrislew
left a comment
There was a problem hiding this comment.
I think having duplicated tags is fine since it doesn't change the logic but could be nice style wise.
3 participants
ArgParser
Changes needed to implement the
traffic_ctlfeature.with_required()method to specify that an option requires anotheradd_example_usage()to support multiple examples per commandtraffic_ctl server debug: new
--append/-aoption when setting tags.Current implementation wipes what's on the debug tags and set the new one, we now have the option to append to the existing tags.
-append/-aoption to append debug tags instead of replacing themwith_required()to enforce--tagsdependencyenable/disablecommandstraffic_ctldocumentationThere are some cleanup as well, as this removes two gold files that aren't required as they are generated by the test.