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

#704: fix settings-security.xml not found #705

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

KianRolf
Copy link
Contributor

@KianRolf KianRolf commented Oct 21, 2024

connected to #704: With this the program successfully locates the settings-security file

@KianRolf KianRolf self-assigned this Oct 21, 2024
@KianRolf KianRolf requested a review from hohwille October 21, 2024 11:17
@coveralls
Copy link
Collaborator

coveralls commented Oct 23, 2024

Pull Request Test Coverage Report for Build 11554748217

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 18 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.007%) to 66.767%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/url/model/file/json/ToolDependencies.java 1 76.92%
com/devonfw/tools/ide/context/IdeContext.java 6 58.9%
com/devonfw/tools/ide/tool/mvn/Mvn.java 11 72.22%
Totals Coverage Status
Change from base Build 11550347943: -0.007%
Covered Lines: 6269
Relevant Lines: 9041

💛 - Coveralls

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.

@KianRolf thanks for your PR. You found the settings.security that is not even documented in the maven-encrpytion-doc. Very nice job for finding this hidden feature what is the major job to solve this story. You also added the config to fix the password encryption. 👍

As I commented, we also need to ensure that we also can decrypt the password when it is to be used. Therefore you need to add this magic system property here:

For a better understanding see also here:

VariableDefinitionString MAVEN_ARGS = new VariableDefinitionString("MAVEN_ARGS", null, c -> c.getMavenArgs(), false, true);

So this way it will be exported as property and work also work if mvn is called directly by the end-user from his shell after initializing with IDEasy so in scenarios such as this:

ide
mvn clean deploy

compared to

ide mvn clean deploy

Since this will only apply once the settings.xml is created, your existing change in Mvn is still needed and therefore fully correct.

To avoid redundancies, you might want to create a method in Mvn creating the -Dsettings.security=... String so it can be re-used from both places. If you want to make this method private what seems to make sense, you could delegate from IdeContext to a new method getMavenArgs() in Mvn that then returns the entire value of MAVEN_ARGS variable and getMavenArgs() in IdeContext is maily just delegating to that method in Mvn. That would be more clean since maven specific logic should be in Mvn rather than IdeContext (see "Separation of Concerns" pattern).

cli/src/main/java/com/devonfw/tools/ide/tool/mvn/Mvn.java Outdated Show resolved Hide resolved
@KianRolf KianRolf requested a review from hohwille October 28, 2024 13:17
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.

@KianRolf thanks for your rework. Now looking perfect to me. 👍
Ready for merge.

@hohwille hohwille added this to the release:2024.10.001 milestone Oct 28, 2024
@hohwille hohwille linked an issue Oct 28, 2024 that may be closed by this pull request
@hohwille hohwille changed the title #1389 Accessing servers via Maven hack #704: fix settings-security.xml not found Oct 28, 2024
@hohwille
Copy link
Member

issue was not added to CHANGELOG but I will fix that after merge so we can progress with the release.
Please double check DoD on next PR.

@hohwille hohwille merged commit 1505bbf into devonfw:main Oct 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

settings-security.xml not found
4 participants