#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.
| detail.HasToken = own.HasToken | ||
|
|
||
| if !detail.HasTokenOwn { | ||
| detail.Endpoint = def.Endpoint |
There was a problem hiding this comment.
This overwrites detail.Endpoint with def.Endpoint whenever the token isn't the profile's own, even though the endpoint above was already correctly set to the profile's own value two lines up. EndpointOwn never gets reset either, so the output shows the default's endpoint labeled as if it were this profile's own.
Trigger: dr --profile staging auth set-url <url> (writes only endpoint), then dr auth profile show staging before running auth login. You'll see the default profile's endpoint, not staging's, with no "(inherited)" tag. A resolver that handles each field independently instead of four hand-copied blocks would make this class of bug structurally harder to write.
| _, scoped := ProfileScopedKeys[key] | ||
| _, owns := owned[key] | ||
|
|
||
| if scoped && activeProfile != "" && !owns { |
There was a problem hiding this comment.
owned here comes from profileSection(activeProfile), which reads process-global viper state, not the file UpdateConfigFile just wrote to disk moments ago in the prior call. For a brand-new profile, ca-cert/ssl_verify can never be considered "owned," so they get dropped from the bare sweep permanently.
Concretely: dr --profile onprem --ca-cert /etc/ssl/onprem-ca.pem auth set-url <url> (your own docs' example) never writes profiles.onprem.ca-cert, which contradicts the comment in auth.go claiming ca-cert persists across invocations. Might be worth resolving ownership from the freshly-written node instead of from viper here.
| 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.
This clears DataRobotURL/DataRobotAPIKey explicitly, but ProfileScopedKeys also has ca-cert/ssl_verify, and neither gets cleared here. A new profile can end up silently using the default profile's CA cert or ssl_verify setting for its first connection, which looks like exactly what this block is trying to prevent for the other two keys.
Might be cleanest to iterate config.ProfileScopedKeys here instead of naming two of its four members.
| // to name one that doesn't exist yet. Clear any inherited credentials so | ||
| // the new profile doesn't silently authenticate against the default | ||
| // profile's instance; the profile's section is created on first write. | ||
| if cmd.Annotations[config.ProfileCreateAnnotationKey] != "" { |
There was a problem hiding this comment.
ProfileCreateAnnotationKey is only set on auth login/auth set-url. Every other command whose PreRunE is EnsureAuthenticatedE (dr start included) fails with "unknown profile" right here, before ever reaching the auto-login flow described two lines up.
Worth deciding if that's intentional (profiles really can only ever be created via login/set-url) or if dr start specifically should carry the same annotation, since it's the advertised first command for a new user.
| rawProfiles := v.GetStringMap(profilesKey) | ||
|
|
||
| names := make([]string, 0, len(rawProfiles)) | ||
| for name := range rawProfiles { |
There was a problem hiding this comment.
This enumerates every key under profiles: with no check against the reserved "default" name, even though ValidateProfileName rejects --profile default elsewhere. A hand-edited profiles.default: {...} section (which write.go is designed to tolerate for arbitrary user edits) would render as a second "default" row in dr auth profile list, permanently unreachable, with nothing ever flagging it.
| v.SetConfigType("yaml") | ||
|
|
||
| err := v.ReadInConfig() | ||
| if err != nil && errors.As(err, &viper.ConfigFileNotFoundError{}) { |
There was a problem hiding this comment.
v.SetConfigFile(configFile) two lines up bypasses viper's findConfigFile(), the only path that actually produces ConfigFileNotFoundError. So this check can't match in practice.
If the file gets deleted mid-session, dr auth profile show/list will surface a raw OS error instead of gracefully falling back to "no profiles configured" the way this is clearly meant to.
| 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! |
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.