#439: refactor tool dependencies and tomcat #515
Merged
Conversation
This file contains 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
…omcat # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/tool/CustomToolCommandlet.java # cli/src/main/java/com/devonfw/tools/ide/tool/Dependency.java # cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java # cli/src/main/java/com/devonfw/tools/ide/tool/PluginBasedCommandlet.java # cli/src/main/java/com/devonfw/tools/ide/tool/androidstudio/AndroidStudio.java # cli/src/main/java/com/devonfw/tools/ide/tool/intellij/Intellij.java # cli/src/main/java/com/devonfw/tools/ide/tool/tomcat/Tomcat.java # cli/src/test/java/com/devonfw/tools/ide/tool/dotnet/DotNetTest.java # cli/src/test/java/com/devonfw/tools/ide/tool/intellij/IntellijTest.java # cli/src/test/java/com/devonfw/tools/ide/tool/tomcat/TomcatTest.java
hohwille
reviewed
Aug 8, 2024
There was a problem hiding this comment.
@aBega2000 as requested I had a look at your PR. It looks already good to me and you are perfectly on the right track 👍
Your changes to use the model with UrlDependencyFile are fully correct, and the way you integrate the environment variable settings is also looking good. I left some review comments for further directions and places to adopt. Please have a look.
cli/src/main/java/com/devonfw/tools/ide/commandlet/AbstractUpdateCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/CustomToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/tool/tomcat/TomcatTest.java
Outdated
Show resolved
Hide resolved
…omcat # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/tool/CustomToolCommandlet.java # cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java # cli/src/test/java/com/devonfw/tools/ide/tool/cobigen/CobigenTest.java
Pull Request Test Coverage Report for Build 10577088575Details
💛 - Coveralls |
hohwille
added
enhancement
hohwille
changed the title
hohwille
requested changes
Aug 13, 2024
There was a problem hiding this comment.
@aBega2000 thanks for your patient work on this complex topic.
You have already gathered very great improvements in this PR and addressed most of the aspects of the story #439. 👍
I left some review comments for rework.
Please, have a look and consider some improvements.
You can also then take this PR out of draft mode.
IMHO we can approve if all review comments are addressed.
Feel free to get in touch if some review comments are unclear for you.
cli/src/main/java/com/devonfw/tools/ide/process/EnvironmentContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/CustomToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/ToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/ToolCommandlet.java
Outdated
Show resolved
Hide resolved
jan-vcapgemini
requested changes
Aug 13, 2024
cli/src/main/java/com/devonfw/tools/ide/process/EnvironmentContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/url/model/file/UrlDependencyFile.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/url/model/folder/UrlEdition.java
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/tool/tomcat/TomcatTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/tool/tomcat/TomcatTest.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/ToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/ToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/ToolCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/ToolCommandlet.java
Outdated
Show resolved
Hide resolved
…ttps://github.com/aBega2000/IDEasy into feature/439-refactor-tool-dependencies-and-tomcat
jan-vcapgemini
requested changes
Aug 14, 2024
cli/src/main/java/com/devonfw/tools/ide/process/EnvironmentContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/url/model/file/UrlDependencyFile.java
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/url/model/file/UrlDependencyFile.java
Show resolved
Hide resolved
jan-vcapgemini
approved these changes
Aug 15, 2024
…ttps://github.com/aBega2000/IDEasy into feature/439-refactor-tool-dependencies-and-tomcat
Closed
Closed
hohwille
reviewed
Aug 22, 2024
cli/src/main/java/com/devonfw/tools/ide/tool/ToolCommandlet.java
Outdated
Show resolved
Hide resolved
hohwille
approved these changes
Aug 22, 2024
There was a problem hiding this comment.
@aBega2000 thank you very much for your work and improvements. You are reaching the finishing line. I resolved all previous review comments that you solved with your rework. There are only very few review remarks to address (currently 5). Great work 👍
Could you have a look and get the last review comments implemented then we can merge this PR and finally close the story.
hohwille
approved these changes
Aug 27, 2024
There was a problem hiding this comment.
@aBega2000 thanks for your excellent and patient rework. You resolved all review comments and brought this to the finishing line. Excellent job 👍
Ready for merge.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes: #439
With this PR I intend to solve the issue of tool-dependencies by refactoring and improving it as much as possible.
As can be seen, many classes are changed and this has happened because of the change in the install method where I now pass EnvironmentContext as argument. Namely this came from the point: "In runTool we can then first create the process context, pass it as EnvironmentContext to the installation method (needs some refactoring) so that there is only one single place in the code getting the dependencies and looping over them that does all together."
The problem at the moment is that some tests are not running successfully anymore. If this approach that I am following is correct, then the last step for this issue would be to fix the mentioned tests so that everything runs smoothly.