Default login api-url to prod#146
Conversation
Remove the interactive API URL selection from `tvc login` and add `--api-base-url` / `TVC_API_BASE_URL` as explicit override paths for internal environments.
There was a problem hiding this comment.
suggestion to do the Config resolution / override up front for clarity
in general, since we now do Clap flag > env > file overrides throughout the SDK, I think we should follow the pattern of
- load config
- resolve config overrides + flags
- pass around now-immutable config to functions
I will also check my #132 to make sure I'm following this pattern
| #[arg(long, env = "TVC_API_BASE_URL", value_name = "URL")] | ||
| pub api_base_url: Option<String>, |
There was a problem hiding this comment.
nit: "BASE_URL" seems redundant to customers?
| #[arg(long, env = "TVC_API_BASE_URL", value_name = "URL")] | |
| pub api_base_url: Option<String>, | |
| #[arg(long, env = "TVC_API_URL", value_name = "URL")] | |
| pub api_url: Option<String>, |
There was a problem hiding this comment.
URL on its own can include paths, query-params, etc. BASE_URL implies that this is a full scheme + host.
| // Load existing config | ||
| let mut config = Config::load().await?; | ||
|
|
||
| // Select or create org | ||
| let (alias, org_config) = select_or_create_org(&mut config, args.org.as_deref()).await?; | ||
| let (alias, org_config) = select_or_create_org( | ||
| &mut config, | ||
| args.org.as_deref(), | ||
| args.api_base_url.as_deref(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
i think code and logic would be much clearer if we resolve the base_url here before going into the select_or_create_org() and nested logic.
Some ideas:
- we could add a field/enum to
tvc::config::turnkey::Configthat defaults toAPI_BASE_URL_PROD, or the actual URL is fine too. Then, pass theconfigintoselect_or_create_org(). That seems clean - if you dont want to do that, call
update_api_base_url_from_override()here instead of within the func, and then you don't have to do it later
either approach reads cleaner to me than the nested if let Some() in current logic
There was a problem hiding this comment.
100% agree. I poked around and tried to make a small change, but a larger refactor in order:
3 participants
Linear: SYS-96
Summary:
tvc loginorgs to prod without prompting for API URL.--api-base-url/TVC_API_BASE_URLas explicit override paths.Tests:
cargo test -p tvc login