Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#280: configure maven properly #305

Merged
merged 34 commits into from
May 16, 2024

Conversation

mvomiero
Copy link
Contributor

@mvomiero mvomiero commented Apr 23, 2024

Fixes: #280

There is a related PR to ide-settings that renames the devon folder to ide, as suggested by @hohwille .
devonfw/ide-settings#45

Note: the configuration will properly work just if the settings are properly updated (or if locally the devon folder in settings is manually renamed to ide)

@coveralls
Copy link
Collaborator

coveralls commented Apr 23, 2024

Pull Request Test Coverage Report for Build 9030688796

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 184 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.5%) to 59.352%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/context/GitContext.java 5 0.0%
com/devonfw/tools/ide/context/IdeContext.java 11 47.22%
com/devonfw/tools/ide/context/GitContextImpl.java 30 48.63%
com/devonfw/tools/ide/context/AbstractIdeContext.java 63 56.91%
com/devonfw/tools/ide/tool/mvn/Mvn.java 75 5.0%
Totals Coverage Status
Change from base Build 9001644591: -0.5%
Covered Lines: 4611
Relevant Lines: 7485

💛 - Coveralls

Copy link
Contributor

@jan-vcapgemini jan-vcapgemini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your addition, finally we can use maven properly :)
I've added some small CR's and I'd suggest to add tests for these features in a new PR as this feature is quite important and needs to be merged asap.

Copy link
Contributor

@jan-vcapgemini jan-vcapgemini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your adjustments. I've added some small suggestions, but think this can go into review now.

@mvomiero
Copy link
Contributor Author

  • I used this PR to update the variables concerning maven in the documentation (variables.adoc) as well. MAVEN_ARGS replaces MAVEN_OPTS as alredy implemented in #212: m2 repo settings as MAVEN_ARGS  #275
  • NOTE: because of the high priority of this PR, unit tests for mvn will be properly developed in a different PR that will follow up.

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvomiero thank you for your PR. Nice work, looks good. 👍
However, I left some review comments for rework to make everything consistent.
Please have a look and rework the commented spots then we can merge.

cli/src/main/java/com/devonfw/tools/ide/tool/mvn/Mvn.java Outdated Show resolved Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/mvn/Mvn.java Outdated Show resolved Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/mvn/Mvn.java Outdated Show resolved Hide resolved
@hohwille hohwille merged commit 5c95a79 into devonfw:main May 16, 2024
4 checks passed
@mvomiero mvomiero deleted the enhancement/280-configureMaven branch June 19, 2024 10:27
@hohwille hohwille added this to the release:2024.05.001 milestone Sep 13, 2024
@hohwille hohwille added the reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented. label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented.
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

configure maven properly
5 participants