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

#131: Support for tool dependencies #145

Conversation

aBega2000
Copy link
Contributor

@aBega2000 aBega2000 commented Nov 20, 2023

Fixes: #131

This is the part of the solution that is implemented in the ToolCommandlet. It automatically detects if there are dependencies to be installed, and installs them in the Repository.

The Json file to be read has the following format:

{
"dependencies": {
"(10.0.27,10.1.18]": [
{
"tool": "java",
"versionRange": "[11,21_35]"
}
],
"(9.0.85,10.0.27]": [
{
"tool": "java",
"versionRange": "[8,11)"
}
]
}
}

A guide to how to create a Json file for dependencies will also be written.

@aBega2000 aBega2000 self-assigned this Nov 20, 2023
@aBega2000 aBega2000 added the bug Something isn't working label Nov 20, 2023
@aBega2000 aBega2000 added this to the release:2024.01.001 milestone Nov 20, 2023
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 PR.
I've found some parts which might be implemented in VersionIdentifier already.
It also looks like the auto reformat (imports) was not applied properly.
Please check.

aBega2000 and others added 4 commits November 23, 2023 00:16
@aBega2000 aBega2000 added enhancement New feature or request software 3rd party software (tools) and removed bug Something isn't working labels Nov 28, 2023
@aBega2000 aBega2000 marked this pull request as ready for review November 28, 2023 08:36
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.
Please check my requested changes. I think we should use the json mapper with a model instead.

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 changes. I've added some small suggestions and comments.
I would also suggest to add some tests for your methods e.g. readJson, findDependencyVersionToInstall etc.

@coveralls
Copy link
Collaborator

coveralls commented Jan 8, 2024

Pull Request Test Coverage Report for Build 8190881140

Details

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

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/os/MacOsHelper.java 1 76.71%
com/devonfw/tools/ide/tool/LocalToolCommandlet.java 71 36.42%
Totals Coverage Status
Change from base Build 8152555067: -0.5%
Covered Lines: 4050
Relevant Lines: 6679

💛 - Coveralls

@tobka777 tobka777 assigned hohwille and unassigned aBega2000 Jan 11, 2024
@jan-vcapgemini
Copy link
Contributor

As Coveralls suggests, we need to add some unit tests for these additions.

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.

@aBega2000 thanks for this PR. You did a good job to implement this rather advanced story and get the pieces together. 👍
As @jan-vcapgemini already requested, you need to add JUnit tests that cover your new code. The tests also help to see the feature in action as then we can look at concrete examples with JSON files used by the test.
Also there are merge issues that have to be addressed (sorry for the review delay).
Please also address the review comments.

this.dependencies = new LinkedHashMap<>();
}

public Map<String, List<DependencyInfo>> getDependencies() {
Copy link
Member

Choose a reason for hiding this comment

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

In general it is better to keep such structures internally to simplify the JSON mapping with Jackson but for the public API instead provide rather something like public List<DependencyInfo> getDependencies(VersionIdentifier toolVersion) or if you want to keep logic out of the data objects and keep them dump and stupid public List<ToolVersionDependencyInfo> getDependencies(). Looking at this method some new member in the team will not have a clue what the semantic of this map will be. What are the keys of type String? Truth is they are VersionIdentifiers (or are they patterns or may they also be version ranges)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my Opinion it also makes sense to only change the String to VersionRange, and that's what I've done. Please let me know if this really makes sense.

Copy link
Member

@hohwille hohwille Mar 19, 2024

Choose a reason for hiding this comment

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

Lets agree on this Map<String, List<DependencyInfo>> as it makes the JSON structure simpler.
But we can omit the root object and you can directly deserialize to Map:
https://www.baeldung.com/jackson-map#bd-3-mapltobjectobjectgt-deserialization

I would propose to keep some class like DependencyJson that contains the code to read the JSON and provides the result as DependencyJson wrapping the Map in Java without the root object being in the JSON syntax.
Then DependencyJson should also implement validation logic so that we can render a warning if we hit something like this:

{ "[1.0.0, 2.0.0]": [ ... ],
  "[1.1.0, 1.9.9]": [ ... ],
  ...
}

So in other words valid JSON should not contain overlapping VersionRanges.

@hohwille hohwille assigned aBega2000 and unassigned hohwille Jan 23, 2024
@hohwille
Copy link
Member

As @jan-vcapgemini already requested, you need to add JUnit tests that cover your new code. The tests also help to see the feature in action as then we can look at concrete examples with JSON files used by the test.
Also there are merge issues that have to be addressed (sorry for the review delay).
Please also address the review comments.

For the record. JUnit test can be found in PR #250

@hohwille
Copy link
Member

All changes had also been included in PR #250 that has already been merged.
Hence, this PR is obsolete and can be closed.

@hohwille hohwille closed this Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request software 3rd party software (tools)
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Support for tool dependencies
4 participants