#826 Added support for multiple site configurations - #885
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 564bda9. Configure here.
carsongee
changed the title
carsongee
changed the title
9dd58bf to
a738762
Compare
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @carsongee What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
Code OwnershipCli Maintainers
Review requested from the teams above. Labels will be removed automatically upon approval. |
|
✅ All smoke tests passed! (Fork PR) ✅ Security Scan: success |
ajalon1
left a comment
ajalon1
left a comment
There was a problem hiding this comment.
Nice feature, and the case-sensitivity fix in the last commit was a good instinct. Went deep on this one and found three real bugs worth fixing before merge (left as inline comments), plus two scope questions I'd like your call on.
Three correctness bugs, see inline comments:
show/cmd.go'sresolveDetail()overwrites a profile's own endpoint with the default's endpoint whenever the profile hasn't set a token yet, without clearing the "own" flag either.- A brand-new profile can never get
--ca-cert/--ssl_verifypersisted into its own section, because the "does this profile already own this key" check reads stale in-memory viper state instead of the file that was just written. - Related: when a new profile is created, only
endpoint/tokenget cleared before the write, notca-cert/ssl_verify. A new profile can silently inherit the default's CA cert for its first connection.
Two things I'd like your call on:
-
SDK/tooling compat. pysdk, rsdk, and dr-agent-cli all read this same
drconfig.yaml, and none of them know aboutprofiles:orDATAROBOT_CLI_PROFILE(checked directly against dr-agent-cli'sdr_config.py, which just doesconfig.get("endpoint")/config.get("token")flat). Switching--profilehas zero effect on any of them when run standalone. Worth at least a documented limitation, and ideally exportingDATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN(rsdk wantsDATAROBOT_API_ENDPOINT) so a shell session can hand these to child tools. -
Active-profile switching. Visibility is already solid:
listmarks the active row andshowsays "(active)", both tested. But there's no persisted "current profile" pointer orusecommand, only the per-invocation flag/env var. gcloud, docker, and kubectl all ship that as core scope, not a follow-up, precisely because silently running against the wrong instance is the most common failure mode of this kind of feature. SinceActiveProfile()would need a real precedence-layer change to support it later, seems worth deciding now rather than by omission.
Two smaller gaps also worth a look (inline): dr start (and every other command whose PreRunE is EnsureAuthenticatedE) fails hard on an unknown --profile instead of reaching the auto-login flow, unlike a brand-new default profile; and a hand-edited profiles.default section is never rejected, so it can silently create a dead, unreachable duplicate row in list.
Smaller things, non-blocking:
LoadProfiles()'serrors.As(err, &viper.ConfigFileNotFoundError{})can't actually match, sinceSetConfigFilebypasses the code path that produces that error. A deleted config file would surface a raw OS error instead of "no profiles configured."- Profile-awareness now lives inside the generic
UpdateConfigFilewrite path, which is whyauth.goneeds two ordered calls with an explanatory comment. An isolated wrapper around it would keep the shared primitive profile-agnostic. list's inline token-status string duplicatesshow'stokenStatus()helper, worth hoisting up to the parent package.resolveDetail's four field-resolution blocks are near-identical hand-copied code. A small data-driven resolver would make the endpoint bug above structurally hard to reintroduce.- Read/write asymmetry on profile-name casing (reads always lowercase, writes preserve existing mixed case) isn't documented or tested.
Happy to pair on any of these.
| printField("endpoint", valueOrDash(d.Endpoint), d.EndpointOwn) | ||
| printField("token", tokenStatus(d.HasToken), d.HasTokenOwn) | ||
| printField("ca-cert", valueOrDash(d.CACert), d.CACertOwn) | ||
| printField("ssl_verify", sslVerifyStatus(d.SSLVerify), d.SSLVerifyOwn) |
There was a problem hiding this comment.
does the CLI actually read ssl_verify anywhere? profile show prints it as a resolved TLS setting and the group help lists it as a profile knob, but setupTLS in cmd/tls.go only reads skip-certificate-check and ca-cert, and i can't find any reader for ssl_verify (no RegisterAlias either).
if it's inert, showing it here reads as "this gates the CLI's TLS" when the real knob is -k / --skip-certificate-check. worth either wiring it in or dropping it from the show output + ProfileScopedKeys and keeping it as a preserved passthrough?
There was a problem hiding this comment.
🤦 good catch. Droppig it for now. I'll take a look at properly wiring it in a follow-up
a738762 to
785c16f
Compare
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @carsongee What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
|
✅ All smoke tests passed! (Fork PR) ✅ Security Scan: success |
|
It should be ready for another look! |
chasdr
left a comment
chasdr
left a comment
There was a problem hiding this comment.
👋 chasbot here. chas is buried in other things so i pulled the re-review shift. came in ready to nitpick and the fixes just hold, which is frankly rude of you.
re-reviewed at HEAD. everything from last round lands:
- resolveDetail no longer shows the default's endpoint wearing a profile's name. credOwn couples endpoint+token now, and there's a regression test on it. 👍
- new-profile
--ca-certactually persists into its own section. profileOwnsKey reads the freshly-written file instead of the startup viper snapshot, and the eu-mtsaas test covers it. - hand-edited
profiles.defaultstops showing up as a phantom second "default" row. - LoadProfiles falls back clean on a deleted config (os.ErrNotExist).
- ssl_verify from my last pass: dropping it as a profile knob and keeping it a preserved passthrough is the right call. nothing in the CLI reads it anyway.
left one non-blocking question inline on the new-profile clear (ca-cert). the two scope calls @ajalon1 raised (other SDKs/plugins not reading profiles:, and no use / persisted active profile) are still yours and his to make, not blockers.
approving. your bugbot can stand down.
| if cmd.Annotations[config.ProfileCreateAnnotationKey] != "" { | ||
| log.Debugf("profile %q does not exist yet; %s is expected to create it", unknownProfile.Name, cmd.CommandPath()) | ||
| viperx.Set(config.DataRobotURL, "") | ||
| viperx.Set(config.DataRobotAPIKey, "") |
There was a problem hiding this comment.
non-blocking: this clears endpoint and token but not ca-cert. the write path is safe now (profileOwnsKey won't persist the default's cert into the new profile's section), so this is only the first login connection running TLS with the default profile's ca-cert. credentials-only on purpose, or iterate config.ProfileScopedKeys here so ca-cert clears too?
3 participants
RATIONALE
Closes #826
Demo
dr-cli-profile-demo.mov
This pull request introduces a new
profilecommand group to the DataRobot CLI, allowing users to inspect and manage multiple named credential profiles for working with different DataRobot installations. It also improves documentation and user guidance for multi-profile usage and enhances the CLI's flexibility for enterprise and multi-environment scenarios.The most important changes are:
New Profile Inspection Commands
auth profilecommand group withlistandshowsubcommands for read-only inspection of named profiles stored indrconfig.yaml. These commands allow users to view all profiles, see which is active, and inspect the resolved settings for each profile, including inherited values. (cmd/auth/profile/cmd.go,cmd/auth/profile/list/cmd.go,cmd/auth/profile/show/cmd.go) [1] [2] [3]cmd/auth/profile/list/cmd_test.go,cmd/auth/profile/show/cmd_test.go) [1] [2]CLI Command Integration and Documentation
profilecommand group in the mainauthcommand, updated the help text to document multi-profile usage, and provided user guidance for selecting profiles via--profileorDATAROBOT_CLI_PROFILE. (cmd/auth/cmd.go) [1] [2]--profileflag to the CLI root command, enabling users to select a named profile for any command invocation. (cmd/root_factory.go)Usability and Consistency Improvements
auth loginandauth set-urlcommands to annotate that they can create a new profile if the specified name does not exist, improving internal consistency and future extensibility. (cmd/auth/login/cmd.go,cmd/auth/seturl/cmd.go) [1] [2]cmd/auth/check/cmd.go)cmd/root_factory.go.om the code andthe review may happen while you are asleep / otherwise not able to respond quickly.
-->
CHANGES
PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: The
run-smoke-testslabel won't work. A required Smoke Tests check will block merge until a maintainer acts:/approve-smoke-teststo run smoke tests (results will set the check)/skip-smoke-teststo bypass the check without running testsPlease comment requesting a maintainer review if you need smoke tests to run.
Note
Medium Risk
Changes authentication config load/save and credential precedence across the CLI, though behavior is heavily tested and env-based auth still overrides profiles.
Overview
Adds named profiles in
drconfig.yamlso one config file can hold credentials for multiple DataRobot installations. Any command can select a profile with--profileorDATAROBOT_CLI_PROFILE; the flag is universal and forwarded to plugins.Config loading merges the active profile’s
endpoint,token,ca-cert, andssl_verifyover the default section (env vars and flags still win). Writes go underprofiles.<name>when a profile is active;auth loginandauth set-urlcan target a new profile name (credentials cleared first so the default instance isn’t reused). New read-onlydr auth profile listanddr auth profile showexpose profiles without printing tokens.Docs and help text are updated for multi-install workflows; debug output redacts nested profile tokens.
Reviewed by Cursor Bugbot for commit 564bda9. Configure here.