#2273: add Git as global tool - #2360
Conversation
8338e9a to
e1ec94b
Compare
Coverage Report for CI Build 33513688801Coverage decreased (-0.02%) to 73.593%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions295 previously-covered lines in 8 files lost coverage.
Coverage Stats💛 - Coveralls |
e1ec94b to
4d0a427
Compare
a072d66 to
ac2e42e
Compare
JoelAdbu
left a comment
There was a problem hiding this comment.
Since Git can now be installed via IDEasy, it would be helpful to improve the error message shown when IDEasy is started without Git being installed.
This change could be implemented in GitContextImpl.java in the findGitRequired() method.
Current message:
String message = "Git " + IdeContext.IS_NOT_INSTALLED_BUT_REQUIRED;Suggested message:
String message = "Git " + IdeContext.IS_NOT_INSTALLED_BUT_REQUIRED +". Try running `ide install git` to install it."cdafd81 to
b5a4595
Compare
JoelAdbu
left a comment
There was a problem hiding this comment.
Thanks for this! ide install git / ide uninstall git is now a first-class global tool, and the overall approach is clean: the requiresVersionResolution() hook to skip repository version resolution for Linux native-package installs, the getInstallerArguments() hook for the Git for Windows silent flags, and the registry DisplayName validation (with proper suffix handling) are all reasonable and well-tested.
CI is green (build, GitGuardian, CLA, coveralls). I don't see any blocking issues — this looks close to ready. A few non-blocking items below, and one behavioral risk worth confirming.
What I verified:
addArgs(List<?>)iterates the list, so.addArgs(getInstallerArguments())is a no-op for the default empty list — no impact on other global tools' installers.- The versionless Linux install path tolerates a
nullresolved version (createToolInstallation/getInstallationPathhandle it), andrequiresVersionResolution()is only false on Linux, so Windows/macOS behavior is unchanged. - New tests extend
AbstractIdeContextTest(AssertJ) and the fixtures match the assertions.
b5a4595 to
a7cd367
Compare
This PR fixes #2273
Implemented changes:
aptandzypper).DisplayName.Testing instructions:
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
build-local-dev.shide install gitand / oride uninstall gitChecklist 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.asciidocChecklist for tool commandlets
Have you added a new
«tool»as commandlet? There are the following additional checks:«tool»«TOOL»_VERSIONand«TOOL»_EDITIONare honored by your commandlet