brbzull0 opened a new issue, #13588:
URL: https://github.com/apache/trafficserver/issues/13588
`ts::ArgParser` lets a command or option declare an environment variable:
```cpp
command1.add_option("--switch", "-s", "switch description", "ENV_VAR", 1,
"default", "lookup_key");
```
The parser reads the variable and stores it, but nothing can ever act on it,
so every
declaration in the tree is decorative. Three separate gaps combine to make
the feature
unusable.
## The env var cannot fill an absent option
`getenv()` is only called from inside the branch that matches the option on
the command
line. If the flag is not typed, the option stays absent and `env()` is
empty, so the
variable cannot supply a parameter the user did not already type:
```
option absent: present=no value='' env=''
```
## The env var cannot fill a missing value
For a fixed-arity option, omitting the value is a usage error raised before
the
environment is consulted:
```console
$ TS_PROBE_ENV=/tmp/from_env.yaml probe config --tags
Error: 1 argument(s) expected by tags
exit=64
```
So for a fixed-arity option there is no reachable state in which the
variable can matter:
with a value it is redundant, without one the parser exits 64. The only
shape where it
could contribute is an option whose value is optional, and
`AT_MOST_ONE_ARG_N` was only
added recently (#13570).
## Nothing reads what is captured
When the option *is* matched with a value, the plumbing does work —
`ArgumentData::env()`
returns the variable in every shape (global option, subcommand option, fixed
arity,
`AT_MOST_ONE_ARG_N`). But `.env()` has no caller in the tree outside
`src/tscore/unit_tests/test_ArgParser.cc`.
Current declarations:
| Declaration | Variable | Effect today |
| --- | --- | --- |
| `traffic_ctl config get/set --cold` | `TS_RECORD_YAML` | none |
| `traffic_ctl --tags` | `TS_DEBUG_TAGS` | none |
| `traffic_ctl --run-root`, `traffic_layout --run-root` | `TS_RUNROOT` |
works, but only because `src/tscore/runroot.cc` calls `getenv` itself |
`TS_RECORD_YAML` is the clearest symptom. `traffic_ctl config get -c` with
no file name is
supposed to be the case the variable serves, and it is ignored in favour of
the compiled-in
config directory:
```console
$ TS_RECORD_YAML=/tmp/coldtest/records.yaml traffic_ctl config get
proxy.config.diags.debug.enabled -c
config get error: bad file: /usr/local/etc/trafficserver/records.yaml #
exit 2
```
`FileConfigCommand` reads only `.value()` and hands the empty string to
`fix_filename()`,
which falls back to `sysconfdir/records.yaml`.
Neither `TS_RECORD_YAML` nor `TS_DEBUG_TAGS` is documented in `doc/`, so no
user-facing
promise is being broken.
`doc/developer-guide/internal-libraries/ArgParser.en.rst` does
document the parameter without noting that the caller must retrieve it by
hand.
## Proposed behaviour
Resolve the variable into the option's values during parsing, so callers
need no special
handling and every declaration means something:
- Precedence is typed value, then environment variable, then declared
default.
- An option that declares a variable accepts a missing value: rather than
exiting 64, it
takes the value from the environment when set, and reports the usage error
only when the
environment is empty too.
- `ArgumentData::env()` keeps returning the raw variable for compatibility.
That makes `traffic_ctl config get -c` honour `TS_RECORD_YAML` and
`traffic_ctl --tags`
honour `TS_DEBUG_TAGS`, with no change to either call site.
### Open question
Should a variable also activate an option that was never typed, as many CLIs
do? It is
convenient but surprising here: exporting `TS_RECORD_YAML` would silently
turn every
`traffic_ctl config get` into a file read rather than an RPC call. The
proposal above
deliberately requires the flag, and this issue is the place to settle it.
## Acceptance criteria
- With the flag typed and no value, the environment variable supplies it,
across fixed
arity, `AT_MOST_ONE_ARG_N`, and the multi-value arities.
- A typed value always wins over the environment variable, which always wins
over the
declared default.
- Usage errors are unchanged when no variable is declared, or when it is
declared and unset.
- Unit tests in `test_ArgParser.cc` covering the precedence chain, and
end-to-end coverage
for `traffic_ctl config get -c` under `TS_RECORD_YAML`.
- `ArgParser.en.rst` states the resolution rules; the `traffic_ctl` appendix
documents both
variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
