Conversation
Coverage Report for CI Build 35987807907Coverage increased (+0.005%) to 74.139%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions37 previously-covered lines in 2 files lost coverage.
Coverage Stats💛 - Coveralls |
d1d4a3e to
4d139f6
Compare
4d139f6 to
5c91172
Compare
ed23f9c to
3f8d714
Compare
|
Thank you for testing this @oanding-blrng and raising the virtualization issue with WSL! After some discussion, we came to the conclusion that it's currently not possible to launch |
maybeec
left a comment
There was a problem hiding this comment.
Thanks for digging into this one 👍 The root-cause analysis is spot on and well documented: Rancher Desktop stopped symlinking into /usr/local/bin and links into ~/.rd/bin since 1.3.0, and IDEasy never looked there. Relaxing RDCTL_CLIENT_VERSION_PATTERN to v? is a good catch, the JavaDoc on resolveRancherDesktopCommand actually explains the why and not just the what, the CHANGELOG entry sits under the correct milestone 2026.08.001, the branch is up to date with main, CI is green and the CLA is signed. Also thanks for the very concrete testing instructions and for the manual-linking recipe in the discussion with @oanding-blrng.
However, there is one change I do not think we can merge as is, plus a set of convention items.
Blocker
1. The new doInstall abort breaks every Linux user who is not on Rancher Desktop (Docker.java:106-115).
The guard is isLinux() && !Files.isDirectory(~/.rd/bin) - it does not check which container runtime is actually in play. This repo explicitly supports Docker Desktop / plain docker on Linux: Docker.getInstalledVersion() has case LINUX -> getDockerDesktopVersionLinux() (Docker.java:149), which greps apt list --installed | grep docker-desktop. For such a user ~/.rd/bin will never exist, so ide docker ps - which worked before this PR - now always aborts with exit code 2. That is a functional regression on a supported platform, which is what coding-conventions.adoc :: Refactorings is about: never silently break something that already shipped.
Two more things fall out of the same block:
GlobalToolCommandlet.doInstallreturns from the package-manager branch (GlobalToolCommandlet.java:131-140) before the already-installed check at:145. So on LinuxdoInstallruns on everyide docker/ide kubectlinvocation - the abort is not a one-off first-install message, it fires on every single call until the GUI has been launched.ide install dockernow exits non-zero even though the installation genuinely succeeded, and theinstallationvalue returned bysuper.doInstall(request)is computed and then thrown away.
The PR description says "Added warning message for first time install" - but the implementation is a hard abort, not a warning. Please make the code match the intent: log a warning (IdeLogLevel.WARNING / LOG.warn, as install() already does for the async-installation case in ToolCommandlet.java:352-358) and return the installation, and scope the check to the case where Rancher Desktop is actually the runtime (e.g. isRancherDesktopInstalled() or the configured edition being rancher). With the ~/.rd/bin fallback in place the subsequent run will then either work or fail with the runtime's own error, instead of IDEasy pre-emptively refusing to run.
Should-fix
-
Windows/macOS are not handled by the fallback.
Files.exists(dir.resolve(command))cannot finddocker.exe. Rancher Desktop creates%USERPROFILE%\.rd\binon Windows as well, so the fallback silently does nothing there.SystemPathalready solves exactly this - seeEXTENSION_PRIORITYandfindBinaryInOrder(SystemPath.java:53,:225-241) - so reusethis.context.getPath().findBinary(Path)instead of a rawFiles.exists. Details inline. -
The resolution logic is duplicated between
Docker.resolveRancherDesktopCommandandKubeCtl.getBinaryName, with two different contracts (nullvs.this.toolas fallback). Duplicated logic across two commandlets drifts apart on the next fix - it should live in one place. -
Magic string literals
".rd"and"bin"in two classes.IdeContext.FOLDER_BINalready exists (IdeContext.java:128) and.rddeserves a documented constant next to it - seecoding-conventions.adoc:: Constants. -
KubeCtl.getInstalledVersion()was not migrated (KubeCtl.java:46-54). It still probes and executesthis.tooldirectly, soide get-version kubectlkeeps missing the~/.rd/binfallback this PR just introduced - the class is left half-migrated. -
No tests. Per
AGENTS.md, a bug fix starts with a failing test. This one is testable without touching the OS:GlobalToolCommandletTestalready exists and even carries a Rancher-flavoured dummy commandlet, andIdeTestContextgives you a controlled user home, so "binary only present in<userHome>/.rd/bin" and "~/.rd/binmissing" are both reproducible and OS-independent. Thev?regex relaxation is likewise a two-line unit test that would pin the bug you fixed. Coveralls reports "no coverable lines changed", which is the same signal. -
getBinaryName()may now return an absolute path, which widens a contract documented as "the name of the binary executable for this tool" (ToolCommandlet.java:143-148). Callers assume a bare name:Path.of(getBinaryName())(ToolCommandlet.java:306,:995,GlobalToolCommandlet.java:221),findBinaryPathByName(...)(:156) andMacOsHelper.findLinkDir(rootDir, getBinaryName())(:579, which doeschild.resolve(tool)inMacOsHelper.java:184). It happens to hold together today, but if we widen the contract we should say so in the base-class JavaDoc so the next person does not break it. -
Manual QA is required before merge. CI cannot exercise any of this: the fallback path, the first-launch message and the symlink setup all depend on a real Rancher Desktop install. Green CI is not sufficient here - please confirm a manual pass on Linux, and at minimum a smoke test on Windows and macOS that the changed
getBinaryName()/getInstalledVersion()paths still behave (Rancher Desktop uses~/.rd/binon macOS too). Also: issue #854 still has no milestone, which the DoD requires before it is closed.
Minor
See the inline comments on naming (String docker holding a runtime, resolveRancherDesktopCommand being called for non-Rancher commands), the possible null command handed to runAndGetSingleOutput, and the magic exit code 2.
Related
@hohwille linked #2171 (RefrEnv) from this issue. That one attacks the same root cause from the other side - refreshing the shell environment after a global install so ~/.rd/bin lands on the PATH properly. Worth a sentence in the PR description on how the two relate, so we do not end up with two overlapping mechanisms.
References: documentation/contributing/coding-conventions.adoc, documentation/contributing/junit-testing.adoc, documentation/contributing/DoD.adoc.
0e0ef16 to
ccc018e
Compare
|
managed to recreate the described scenario and got the following warning as output: Rancher Desktop has been installed but not launched yet. Please start Rancher Desktop once so that it sets up its command-line tools (docker, kubectl, ...) in /root/.rd/bin, then re-run your command. |
da9b2ab to
dad1b7d
Compare
# Conflicts: # cli/src/main/java/com/devonfw/tools/ide/tool/docker/Docker.java
# Conflicts: # cli/src/main/java/com/devonfw/tools/ide/tool/docker/Docker.java
# Conflicts: # cli/src/main/java/com/devonfw/tools/ide/tool/docker/Docker.java
61fdbde to
7715b3a
Compare
This PR fixes #854
Implemented changes:
~/.rd/bin.Clarification: Rancher Desktop creates
~/.rd/binand links its CLI tools (docker, kubectl, rdctl) there only after its first GUI launch. Therefore in order to launch docker or kubectl using IDEasy, we added a warning message for when the user tries to launch it without having launched the GUI first.Testing instructions
ide kubectland verify that the installation is successful that the warning appears.ide kubectlagain and verify that it runs successfully.docker.Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal