-
Notifications
You must be signed in to change notification settings - Fork 19
Use OXIDE_PROFILE env var to select profile #1237
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
Conversation
It can be convenient to select the silo to target via the environment. We have the `OXIDE_HOST` and `OXIDE_TOKEN` env vars today, but these require putting your private token in the environment. Add a new `OXIDE_PROFILE` env var which accepts just the profile name from `credentials.toml`. To avoid confusion around precedence between this and `OXIDE_HOST`, return an error if both are set. Closes #1080
This conforms to the chart in #1080 (comment), with the exception that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Do we want to add documentation... somewhere? Perhaps in the help output or in cli/README.md? I'm not sure if we want that table from #1080 exactly or just something like that.
if std::env::var("OXIDE_HOST").is_ok() && std::env::var("OXIDE_PROFILE").is_ok() { | ||
return Err(OxideAuthError::HostProfileConflict); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could do something with clap's parsing to examine env vars... but I'm not sure that would be better; just noting it in case it seems worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about adding OXIDE_PROFILE
to just the CLI, which would let easy to do with Arg::env, as you say. This seems like something that would be generally convenient, so I went with including it in the SDK.
&ClientConfig::default().with_host_and_token(&host_env, &token_env), | ||
) | ||
.expect("client authentication from host/token failed"); | ||
)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the new behavior in these cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously this path was in practice infallible, now it can fail if OXIDE_HOST
and OXIDE_PROFILE
are both set. With the expect
we get a somewhat confusing panic here:
$ OXIDE_HOST=a OXIDE_TOKEN=b OXIDE_PROFILE=c cargo run -- auth status
thread 'main' panicked at cli/src/cmd_auth.rs:501:14:
client authentication from host/token failed: HostProfileConflict
Co-authored-by: Adam Leventhal <[email protected]>
Co-authored-by: Adam Leventhal <[email protected]>
It can be convenient to select the silo to target via the environment. We have the
OXIDE_HOST
andOXIDE_TOKEN
env vars today, but these require putting your private token in the environment.Add a new
OXIDE_PROFILE
env var which accepts just the profile name fromcredentials.toml
. To avoid confusion around precedence between this andOXIDE_HOST
, return an error if both are set.Closes #1080