[CFX-6634] fix(update): prevent circular dr symlink#605
Conversation
ajalon1
changed the title
|
Putting this out here, but it is untested. If someone wants to attempt to test this, they can tag a new dev release from this branch, let the publish workflow complete, and then do a |
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
❌ Some smoke tests failed. (Fork PR) ❌ Security Scan: failure |
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
❌ Some smoke tests failed. (Fork PR) ❌ Security Scan: failure |
In DataRobot Codespaces dr is installed as a directory on PATH (dr/dr),
not a flat file. install.sh's `ln -sf dr datarobot` followed the
datarobot->dr directory symlink on the second `dr self update` and wrote
the link inside it, turning dr/dr into a self-referential `dr -> dr`
symlink ("too many levels of symbolic links").
- install.sh: add ensure_datarobot_alias() using `rm -f` + `ln -sfn`
(no-dereference) so ln never descends into a symlinked directory.
- cmd/self/update: pass INSTALL_DIR resolved from the running binary so
updates land where dr actually lives instead of ~/.local/bin.
- Add alias regression smoke test and resolveInstallDir unit tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c-h-russell-walker
left a comment
c-h-russell-walker
left a comment
There was a problem hiding this comment.
Very nice - looks great 👍
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 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 |
3 participants
RATIONALE
In DataRobot Codespaces, running
dr self updatetwice leftdra self-referential symlink (dr → dr), breaking the CLI with "too many levels of symbolic links". Codespaces installdras a directory onPATH(dr/dr) rather than a flat file;install.sh'sln -sf dr datarobotthen followed thedatarobot → drdirectory symlink on the second run and wrote the new link inside the directory, clobbering the real binary. This is critical (CFX-6634) since the CLI becomes unusable, and reproduces only in Codespaces.CHANGES
install.sh: addensure_datarobot_alias()usingrm -f+ln -sfnsolnnever dereferences a symlinked directory.cmd/self/update: passINSTALL_DIRresolved from the running binary so updates land wheredractually lives.resolveInstallDirunit tests and aninstall.shalias regression smoke test.NOTES
main(dr self updatefetchesinstall.shfrommain); the Go change ships in the next build.TESTING
task lint(0 issues),go test -race ./cmd/self/..., install alias + install integration smoke tests all pass.PR Automation
Important
Forked PR: the
run-smoke-testslabel won't work — a maintainer needs to/approve-smoke-testsor/skip-smoke-teststo clear the required Smoke Tests check.Note
Medium Risk
Touches self-update and install scripting paths that can brick the CLI if wrong, but changes are narrow and covered by unit and smoke tests.
Overview
Fixes a Codespaces-only failure where a second
dr self updatecould replace the real binary with a self-referentialdr → drsymlink (“too many levels of symbolic links”).install.shcentralizes alias creation inensure_datarobot_alias(): remove any existingdatarobotentry, thenln -sfnsolndoes not follow adatarobot → drdirectory symlink and write a link insidedr/dr. The script can be sourced for tests viaDR_INSTALL_SH_NO_MAIN.dr self updatesetsINSTALL_DIRfrom the running binary’s directory (symlinks resolved) when the user has not setINSTALL_DIR, so the fetched installer updates in place instead of defaulting to~/.local/bin.Adds Go tests for
resolveInstallDirand a shell smoke test for the alias behavior in Codespace vs flat layouts.Reviewed by Cursor Bugbot for commit 24ebb90. Configure here.