[CFX-5789] [CFX-5790] ToolRegistry & Surface environment-aware failure messaging#577
[CFX-5789] [CFX-5790] ToolRegistry & Surface environment-aware failure messaging#577taras-pokornyy wants to merge 6 commits into
ToolRegistry & Surface environment-aware failure messaging#577Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| fmt.Fprint(w, msg) | ||
|
|
||
| return installed, fmt.Errorf("install failed for %q (exit code %d)", prerequisite.Name, exitCode) |
There was a problem hiding this comment.
Start flow hides install failures
High Severity
Detailed install failure text is written only to the io.Writer, while the returned error is a short exit-code message. The quickstart TUI installs deps into a buffer and on failure displays only m.err, so users lose tips, the tried command, and streamed command output that this change added.
Triggered by project rule: Bugbot Rules for DataRobot CLI
Reviewed by Cursor Bugbot for commit 29b422a. Configure here.
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6c10ba7. Configure here.
| // Unix-like systems (Linux & macOS) return 126 when a file lacks execute permissions | ||
| if (runtime.GOOS == "linux" || runtime.GOOS == "darwin") && exitCode == 126 { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Exit 126 ignored on Windows
Medium Severity
Permission-denied handling treats exit code 126 only when runtime.GOOS is linux or darwin. Installs still run via sh -c on Windows (e.g. Git Bash), where 126 can mean “not executable,” but without matching stderr text the failure is not classified as permission denied and users miss the Administrator-oriented tip.
Additional Locations (1)
Triggered by project rule: Bugbot Rules for DataRobot CLI
Reviewed by Cursor Bugbot for commit 6c10ba7. Configure here.
| func (fs FallbackStrategy) getStrategyTip(goos string) string { | ||
| cmds := fs.Commands | ||
|
|
||
| if goos == "windows" && len(fs.CommandsWindows) > 0 { |
There was a problem hiding this comment.
nit here:
Do we have reusable values in this repo. so we're not using magic strings for things like "windows" and likely "darwin" and "linux" ?
There was a problem hiding this comment.
"windows", "darwin", and "linux" are all potential values of runtime.GOOS, which is a build-time constant.
https://pkg.go.dev/internal/goos#GOOS
https://cs.opensource.google/go/go/+/refs/tags/go1.26.4:src/internal/goos/zgoos_linux.go;l=7
I am not recommending this because I'm not convinced it's worth it here but one way to approach this would be to conditionally compile the fallbackstrategy code using go build. (You can extend packages based on runtime.GOOS and runtime.GOARCH.) A working example of this can be found in internal/workload where Wojciech has implemented OS-specific diskspace implementations.
- https://github.com/datarobot-oss/cli/blob/main/internal/workload/sync/diskspace_unix.go
- https://github.com/datarobot-oss/cli/blob/main/internal/workload/sync/diskspace_windows.go
What MIGHT be worth doing is something like this, where we enumerate the OS we actually support.
type OS string
const (
OSDarwin OS = "darwin"
OSLinux OS = "linux"
OSWindows OS = "windows"
)
func IsWindows(os OS) bool { return os == OSWindows }
func IsUnixLike(os OS) bool { return os == OSDarwin || os == OSLinux }
There was a problem hiding this comment.
This is all just informational though, since I know @taras-pokornyy you want a bigger discussion on how we can scale this registry.
| return "" | ||
|
|
||
| case 1: | ||
| return " Try: " + cmds[0] |
There was a problem hiding this comment.
So wasn't sure where to put this.
Curious what you and @ajalon1 and others think.
I like your usage of \t elsewhere and wonder if that would be better to use than the many places you're using double spaces??
| return " Try: " + cmds[0] | |
| return "\tTry: " + cmds[0] |
To me it's:
- less likely to accidentally have one or three spaces at some point as a UI bug
- easier to read in the code as opposed to empty spaces - maybe that's just me though
There was a problem hiding this comment.
I think it'd be good to stick to a single convention, but that becomes a renderer / UI issue and I think we can do this better with a more structured formatting approach than just \t. text/tabwriter is a helpful package if we want to go that route.
c-h-russell-walker
left a comment
c-h-russell-walker
left a comment
There was a problem hiding this comment.
Looks great - made some nit comments. And thanks for the demo today during our meeting.
ajalon1
left a comment
ajalon1
left a comment
There was a problem hiding this comment.
This LGTM as is. The registry could use some more work, but that is out of scope.
The one thing I would suggest addressing is what happens with strategy.WithVersion(""). You can either do that here or in a followup.
| nvmDir := getenv("NVM_DIR") | ||
|
|
||
| if nvmDir == "" { | ||
| home := getenv("HOME") |
There was a problem hiding this comment.
The correct way to do this would be home, err := os.UserHomeDir()
| func (fs FallbackStrategy) getStrategyTip(goos string) string { | ||
| cmds := fs.Commands | ||
|
|
||
| if goos == "windows" && len(fs.CommandsWindows) > 0 { |
There was a problem hiding this comment.
"windows", "darwin", and "linux" are all potential values of runtime.GOOS, which is a build-time constant.
https://pkg.go.dev/internal/goos#GOOS
https://cs.opensource.google/go/go/+/refs/tags/go1.26.4:src/internal/goos/zgoos_linux.go;l=7
I am not recommending this because I'm not convinced it's worth it here but one way to approach this would be to conditionally compile the fallbackstrategy code using go build. (You can extend packages based on runtime.GOOS and runtime.GOARCH.) A working example of this can be found in internal/workload where Wojciech has implemented OS-specific diskspace implementations.
- https://github.com/datarobot-oss/cli/blob/main/internal/workload/sync/diskspace_unix.go
- https://github.com/datarobot-oss/cli/blob/main/internal/workload/sync/diskspace_windows.go
What MIGHT be worth doing is something like this, where we enumerate the OS we actually support.
type OS string
const (
OSDarwin OS = "darwin"
OSLinux OS = "linux"
OSWindows OS = "windows"
)
func IsWindows(os OS) bool { return os == OSWindows }
func IsUnixLike(os OS) bool { return os == OSDarwin || os == OSLinux }
| return "" | ||
| } | ||
|
|
||
| return strategy.withVersion(prerequisite.MinimumVersion).getStrategyTip(goos) |
There was a problem hiding this comment.
What happens if prerequisite.MinimumVersion == ""? Will we get a string like "nvm install {version}" back?
| func (fs FallbackStrategy) getStrategyTip(goos string) string { | ||
| cmds := fs.Commands | ||
|
|
||
| if goos == "windows" && len(fs.CommandsWindows) > 0 { |
There was a problem hiding this comment.
This is all just informational though, since I know @taras-pokornyy you want a bigger discussion on how we can scale this registry.
| } | ||
|
|
||
| // isPermissionDenied inspects exit codes and stderr text across OS types. | ||
| func isPermissionDenied(exitCode int, stderr string) bool { |
| return "" | ||
|
|
||
| case 1: | ||
| return " Try: " + cmds[0] |
There was a problem hiding this comment.
I think it'd be good to stick to a single convention, but that becomes a renderer / UI issue and I think we can do this better with a more structured formatting approach than just \t. text/tabwriter is a helpful package if we want to go that route.
3 participants
RATIONALE
When
dr deps installruns a tool's install command and it fails, the previous error was a bare exit-code line with no guidance on what to try next. This change wires up the existingToolRegistrydata to produce actionable, environment-aware tips when an install command fails — "You have pyenv — try: pip install uv" — and falls back to the canonical install command (e.g. curl | sh) when no manager is detected. Permission-denied failures (exit code 126 or matching stderr text) get an OS-specific "re-run with sudo / as Administrator" message instead.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
Low Risk
UX and messaging only around failed installs; success path unchanged. Suggestions are advisory shell commands, not auto-executed.
Overview
When
dr deps installfails, users now get structured output on the writer (not only a minimal error): what was tried, optional tips, retry command, and docs URL.InstallPrerequisitestees install output into a buffer so failures can be classified and messaged.A new
ToolRegistrymaps tools (python, uv, node, pulumi, task, git) to ordered install strategies (pyenv/brew/asdf/winget/etc.) plus fallbacks.DetectEnvironmentprobes PATH and NVM layout;selectInstallStrategypicks the first matching manager, skipping the manager that just failed.buildInstallFailureMsg/buildInstallTipadd OS-specific guidance for permission errors (exit 126 / stderr heuristics) vs alternate install commands (e.g. “You have pyenv — try: pip install uv”). Returned errors are shortened; detail lives in stdout.Broad unit coverage was added for registry, strategy selection, and failure messaging.
Reviewed by Cursor Bugbot for commit 6c10ba7. Configure here.