Fix: Clean up command line directory inconsistencies - #1079
Conversation
| if (prefixDir.empty()) { | ||
| prefixDir = bsl::getenv("BMQ_PREFIX"); | ||
| } |
There was a problem hiding this comment.
I guess the only real question here is should the env var win even if --prefixDir is provided on the cli? Unsure.
There was a problem hiding this comment.
Usually the order of precedence I expect is config file, CLI arg, environment variable in order of lowest to highest precedence (i.e. BMQ_PREFIX should win)
There was a problem hiding this comment.
I have no strong opinions either way.
With bmqtool, it looks like the CLI param --broker overrides BMQ_PORT:
But
BMQ_BROKER_URI overrides the user's session options:
So it's a mess even with the only two examples we have.
There was a problem hiding this comment.
Did a little research, it seems that most other CLI tools have CLI arg as the highest precedence.
- Docker: https://docs.docker.com/reference/cli/docker/#configuration-files "Command line options override environment variables and environment variables override properties you specify in a config.json file."
- Git: Does not document this very clearly from what I can tell (https://git-scm.com/docs/git#_environment_variables). But, running the following test confirms that the CLI argument takes higher precedence:
cd ~ GIT_DIR=~/blazingmq/.git git --git-dir=~/blazingmq-sdk-java/.git fetch # remote: Enumerating objects: 23, done. # remote: Counting objects: 100% (23/23), done. # remote: Compressing objects: 100% (9/9), done. # remote: Total 23 (delta 8), reused 13 (delta 8), pack-reused 0 (from 0) # Unpacking objects: 100% (23/23), 5.93 KiB | 276.00 KiB/s, done. # From https://github.com/bloomberg/blazingmq-sdk-java # 9203baf..a342729 main -> upstream/main
- HashiCorp Vault: I think this is a bit more like BlazingMQ as a network server. https://developer.hashicorp.com/vault/docs/commands#configure-environment-variables "You can use environment variables to configure the CLI globally. Some configuration settings have a corresponding CLI flag to configure a specific command. For example, export
VAULT_ADDR='http://localhost:8200'sets the address of your Vault server globally, while-address='http://someotherhost:8200'overrides the value for a specific command."
I have not been able to find anything that follows the precedence config file < CLI arg < environment variable.
Second thing I've found is it's hard to find this documented explicitly. So I think we should document explicitly what we choose.
There was a problem hiding this comment.
Coming back to this after two months but I have no idea what I was thinking when I wrote this originally. Most tools I use do the config file < environment var < CLI arg order. Maybe I was thinking about how one might control broker configurations from a docker image?
Either way I do think the most important thing is to document the precedence regardless of what takes precedent. I just don't agree with what I originally wrote anymore.
711933f to
42f402d
Compare
The `--config` command line argument was parsed as optional, but if it was not passed to the broker, we would error out and fail. This patch makes the command line argument required during parsing. Signed-off-by: Patrick M. Niedzielski <pniedzielski@bloomberg.net>
Signed-off-by: Patrick M. Niedzielski <pniedzielski@bloomberg.net>
42f402d to
8046e63
Compare
|
See above comment ^ |
We noticed two inconsistencies while reviewing the
bmqbrkrmain function:--configwas parsed as an optional argument, but if it was not given on the command line, we would detect and fail after parsing. We can just make it required.All environment variables that the BlazingMQ broker knows about can be overridden by command line arguments, except
BMQ_PREFIX. We can add a new command line argument to do that.