#2251: add uninstall support for global tools windows - #2301
Open
cap-juan wants to merge 3 commits into
Open
Conversation
cap-juan
force-pushed
the
feature/2251-uninstall-global-tools-windows
branch
2 times, most recently
from
August 10, 2026 12:52
86cac90 to
15b6180
Compare
cap-juan
changed the title
cap-juan
added
enhancement
cap-juan
force-pushed
the
feature/2251-uninstall-global-tools-windows
branch
from
August 10, 2026 12:58
15b6180 to
cbae6d3
Compare
Collaborator
Coverage Report for CI Build 31963401188Warning No base build found for commit Coverage: 72.887%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
hohwille
changed the title
cap-juan
force-pushed
the
feature/2251-uninstall-global-tools-windows
branch
from
August 11, 2026 07:41
cbae6d3 to
bab534c
Compare
krystynaShatkovska
self-requested a review
August 13, 2026 10:11
Contributor
|
Good work. All unit tests are passed. Could not test with pgAdmin installation as it does not work in my enviroment |
JoelAdbu
reviewed
Aug 14, 2026
JoelAdbu
left a comment
JoelAdbu
left a comment
Contributor
There was a problem hiding this comment.
I also tested the installation and uninstallation of pgAdmin. Everything works perfectly. Well done!
hohwille
approved these changes
Aug 18, 2026
| private String findUninstallKey(String appName) { | ||
|
|
||
| for (String registryBasePath : REGISTRY_BASE_PATHS) { | ||
| LOG.warn("Searching registry for {} in {}", appName, registryBasePath); |
Member
There was a problem hiding this comment.
This is not a reasonable warning. Was that added for debugging and accidentally committed?
Otherwise this should be debug or trace.
Comment on lines
+264
to
+283
| WindowsHelperImpl helper = new WindowsHelperImpl(context) { | ||
|
|
||
| @Override | ||
| public WindowsAppInstallation getAppInstallationFromRegistry( | ||
| String requestedAppName) { | ||
|
|
||
| assertThat(requestedAppName).isEqualTo(appName); | ||
|
|
||
| return new WindowsAppInstallation( | ||
| "1.0", | ||
| null, | ||
| uninstallString, | ||
| "C:\\Program Files\\Test"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void executeUninstallCommand(String command) { | ||
| capturedCommand[0] = command; | ||
| } | ||
| }; |
Member
There was a problem hiding this comment.
Instead of putting multiple redundant such anonymous classes into tests, you should simply extend WindowsHelperMock properly. I see two options:
- override
uninsallApplicationto simply remove the app fromregistryMap. - override the
executeUninstallCommandto capture the executed uninstall command without actually executing it and give a way to access it from tests e.g. via getter or even customassertThatlike we did for our logs. - do not override anything and just register a test application with uninstall command
echo test application has been uninstalledso you can assert thattest application has been uninstalledwas logged.
| * https://github.com/devonfw/IDEasy/issues/2131[#2131]: Improve `ide upgrade --mode=` auto-completion | ||
| * https://github.com/devonfw/IDEasy/issues/1870[#1870]: Add generic get-version implementation for global tools under windows | ||
| * https://github.com/devonfw/IDEasy/issues/1558[#1558]: Added installation log information to the "Select Project Folder" and exit dialogs and enhanced `windows-installer/README.adoc`. | ||
| * https://github.com/devonfw/IDEasy/issues/2251[#2251]: Provide generic uninstall support for globally installed tools (windows) |
Member
There was a problem hiding this comment.
Sorry for the late review. You need to move this up to the current release.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #2251
Implemented changes:
Testing instructions
./build-local-dev.sh.ide install pgAdmin.ide -d uninstall pgadmin.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.batand notfeature/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
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc