#861/#1688: Add functionality to global tool commandlets for installation and uninstallation under windows and linux - #1862
Conversation
…install on windows
…nd uninstall to abstract
…-installation-of-pgadmin # Conflicts: # CHANGELOG.adoc
…-installation-of-pgadmin # Conflicts: # CHANGELOG.adoc
jakozian
added
windows
Coverage Report for CI Build 25037187462Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.4%) to 70.272%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions205 previously-covered lines in 7 files lost coverage.
Coverage Stats
💛 - Coveralls |
jakozian
added
docker
MarvMa
left a comment
MarvMa
left a comment
There was a problem hiding this comment.
You found a really smart way of uninstalling Tools via Win Registry, Nice Work 👍 I left some comments regarding readability and maintainability.
| if (this.context.isForceMode()) { | ||
| // has to be set to null so the already installed tool doesn't open | ||
| // during the installation process in force mode | ||
| installationPath = null; |
There was a problem hiding this comment.
Using null here as a control signal to prevent the tool from being started makes the flow harder to understand.
It might be safer and clearer to introduce an explicit flag (e.g. startTool / skipStart) or a small result type to model this state explicitly and avoid the usage of null as a flag.
There was a problem hiding this comment.
You are absolutely right! Changing that...
There was a problem hiding this comment.
This filecurrently mixes several responsibilities (error handling, force‑mode behavior, user messaging, and installation result creation).
Splitting the implementation into smaller more focused methods could improve readability and make the control flow easier to understand and test.
There was a problem hiding this comment.
I agree with you on that but as @hohwille suggested this would not be part of this issue.
| LOG.warn("The tool {} is about to be installed. Please complete the installation and if required " | ||
| + "reboot your machine. Then run the command to start the tool.", this.tool); | ||
| } else { | ||
| throw new CliException("The tool " + this.tool + " is about to be installed. Please complete the installation and if required " |
There was a problem hiding this comment.
The exit code 2 is not clear here.
Introducing a named constant or enum (e.g. EXIT_USER_ACTION_REQUIRED) would make the intention clearer and improve maintainability/readability.
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@jakozian thanks for your PR. I really appreciate that you found a solution how to find the version of any installed software on Windows from the registry. That is great progress. Also #1688 is easily solved by your PR. 👍
However, after looking at all the details, I got the impression that we went the wrong way. This PR is getting more and more complex since you seem to have hit a pitfall.
GlobalToolCommandlet is a mess and that is not your fault. I was not aware of that and solving this is not an easy task.
Therefore I would go back and suggest the following steps as individual PRs:
- PR fixing only #1688 by just removing the unwanted
LOG.errormessages you also removed here. IMHO that is all we need to implement that story (besides a CHANGELOG entry). - PR to improve our WindowsHelper with ability to determine installed version of an installed tool from Windows registry. Move your changes to
WindowsHelper*into that PR and try to improve your implementation addressing my review concern. Maybe by asking copilot or some other AI you can find even a direct query method to search the key where DisplayName matches a given prefix. - When all this is merged, lets stepwise improve
GlobalToolCommandletand its sub-classes. Maybe we will even split this into multiple PRs e.g. per OS. The closer we look, the more we will find. I also just noticed, that whenpodmanis installed, this will be used as an alias fordockerwhat totally makes sense. But in such case the edition would rather bepodmanand the version is yet fully unclear. Also for Linux I see tons of improvements required since things are not working on non Debian based distributions, etc.
Mac is not yet supported by this at all.
Sorry, that I did not see this upfront. But if we keep trying to make this perfect in this single PR we will both not have fun. So lets go for divide & conquer what should always be the pattern to solve complex problems.
| String displayVersion = windowsHelper.getRegistryValueBySearch(getWindowsAppName(), "DisplayVersion"); | ||
| return VersionIdentifier.of(displayVersion); | ||
| } else if (this.context.getSystemInfo().isLinux()) { | ||
| String output = this.context.newProcess().runAndGetSingleOutput(getBinaryName(), "version"); |
There was a problem hiding this comment.
This does not look like a reliable way to determine the version:
$ pgadmin4 version
bash: pgadmin4: command not found
$ docker version
Client:
Version: 29.1.4-rd
API version: 1.52
Go version: go1.25.5
Git commit: 3c6914c
Built: Fri Jan 9 20:47:42 2026
OS/Arch: windows/amd64
Context: default
Server:
Engine:
Version: 29.1.3
API version: 1.52 (minimum version 1.41)
Go version: go1.25.5
Git commit: fbf3ed25f893e6ce21336f1101590e40a13934f4
Built: Sun Dec 14 05:31:58 2025
OS/Arch: linux/amd64
Experimental: false
containerd:
Version: v2.2.0
GitCommit: 1c4457e00facac03ce1d75f7b6777a7a851e5c41
runc:
Version: 1.4.0
GitCommit: 8bd78a9977e604c4d5f67a7415d7b8b8c109cdc4
docker-init:
Version: 0.19.0
GitCommit:
You would rather need to ask the package manager for the version.
This is not trivial to implement since e.g. on Ubuntu you would install with
sudo apt install pgadmin4-desktop
But to determine the version you would rather do:
dpkg -s pgadmin4-desktop | grep '^Version:'
I just noticed that on Linux we currently install and uninstall pgadmin4 in a very odd and crazy way.
According to https://www.pgadmin.org/download/pgadmin-4-apt/ we should just do sudo apt install pgadmin4-desktop to install it.
My vision was that if we have such reasonable installation commands, we could derive the command to determine the version automatically from that.
Obviously if we do such crazy stuff this does not seem to be possible.
Still my approach would be to implement the automatically generated command to determine the version on Linux in a protected method based on the PackageManagerCommand.
IMHO also the uninstallation could be automatically derived from the installation PackageManagerCommand.
Only in edge-cases, we could then override the protected method if the default does not work.
And how about Mac?
To me it seems the entire "world" of GlobalToolCommandlet is more in a messy alpha/beta state but something reasonable we crafted by design.
|
|
||
| Pattern pattern = Pattern.compile(displayNameRegex, Pattern.CASE_INSENSITIVE); | ||
| for (String path : REGISTRY_PATHS) { | ||
| List<String> output = runReg("query", path, "/s"); |
There was a problem hiding this comment.
This is giving me the details of all installed software.
On my machine ~4000 lines of text.
That seems like a very inefficient way to solve this.
The idea of a registry query is to select only the data we really need.
A first improvement would be to run
reg query path /S /v DisplayName
BTW: it is also slightly confusing that you allow to pass any value for the 2nd parameter key but name the first parameter displayNameRegex so you actually assume that the key is DisplayName.
It can be a good idea to generalise a method to allow more flexibility.
On the other hand then you also need to implement it in a way that it solves all the potential cases properly.
I would recommend to focus on creating a method dedicated to determine the version of a product given the (prefix of) the DisplayName.
| ProcessResult result = this.context.newProcess().errorHandling(ProcessErrorHandling.LOG_WARNING).executable("reg").addArgs("query", path, "/v", key) | ||
| .run(ProcessMode.DEFAULT_CAPTURE); | ||
| if (!result.isSuccessful()) { | ||
| return null; | ||
| } | ||
| List<String> out = result.getOut(); | ||
| return retrieveRegString(key, out); | ||
| List<String> out = runReg("query", path, "/v", key); | ||
| if (out != null) { | ||
| return retrieveRegString(key, out); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Great that you extracted the call of reg as process for reuse via method runReg.
By here you are now calling it twice:
- in line 98/99 there is the leftover of the original call.
- In line 103 you call it again.
You should remove lines 98-102 that do not make sense any more.
|
BTW: If you would put breakpoints into Then the existing |
|
Closing due to too complex implementations. |
4 participants
This PR fixes #861 and #1688
Implemented changes:
uninstall,get-versionandget-editionfor global tools(With this generic global tool logic the exception thrown while installing pgadmin is fixed #861)
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