#1953: Complete cleanup commandlet implementation and review handoverShow more lines - #2230
#1953: Complete cleanup commandlet implementation and review handoverShow more lines#2230Caylipp wants to merge 42 commits into
Conversation
…p-commandlet-handover
- scan all global software repositories - detect used versions by resolved installation paths - support links to installation subfolders and nested extra tools - make cleanup discovery stateless - track project usage only on version level - reuse the global force mode for confirmation - add regression tests for used and unused installations
Coverage Report for CI Build 30885837220Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 72.825%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions125 previously-covered lines in 10 files lost coverage.
Coverage Stats
💛 - Coveralls |
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
We are closing the finishing line 👍
Still I found some points...
There was a problem hiding this comment.
Thanks for picking this up the substance of the #1957 review is addressed and the new tests cover exactly the cases that were requested there. I went through the branch and found a few points before this can go in, three of them blocking from my side:
- the new tests error out on Windows without symlink privileges (verified locally, 3 of 4 tests)
- the CHANGELOG entry landed in an already released section
- the confirmation cannot actually be skipped with
-falone
The remaining ones are smaller correctness and consistency points. I left out anything already covered by the review above (common super class for the model classes, delete flag only on version level, central findProjects()).
- add common abstract base class for installed software items - centralize software item name and path - rename cleanup model classes for reuse
- keep deletion state only on software versions - delete unused versions before empty parent folders - remove redundant deletion state from tools and editions
- add reusable project discovery to IdeContext - exclude non project folders centrally - reuse project discovery in cleanup and GUI
- scan default, Maven, and configured custom repositories explicitly - update cleanup test for the configured custom repository
Co-authored-by: quando632 <quang-hieu.do@capgemini.com>
- add better help message - propagate user cancellation through the standard abort handling - correct software discovery variable names - report affected tools and editions accurately - cover batch force mode and Windows symlink restrictions - adjust CHANGELOG.adoc entry
quando632
left a comment
quando632
left a comment
There was a problem hiding this comment.
All eight points from my previous review are addressed on dc95fdea. mvn -Dtest=CleanupCommandletTest test is green (5 tests, 3 skipped for missing symlink privileges), and checkstyle and spotless report no violations in the new files. Solving the counter problem by reporting "affected" tools and editions is nicer than what I suggested.
Two issues came out of the changes themselves, one of them blocking. Both only appear when the individual fixes are combined, so they are not visible in any single change.
…anupCommandlet.java Co-authored-by: quando632 <quang-hieu.do@capgemini.com>
…anupCommandlet.java Co-authored-by: quando632 <quang-hieu.do@capgemini.com>
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@Caylipp thanks for taking up this story and PR. 👍
I still have suggestions for improvement.
I hope you can address them tomorrow and I can merge the next day what would be awesome.
If you are running out of time, you may skip some "nice to haves" and I will create a cleanup issue and merge then. Thanks.
| for (Path ideasyProject : ideasyProjects) { | ||
| String projectName = ideasyProject.getFileName().toString(); | ||
| Path ideasyProjectSoftware = ideasyProject.resolve(IdeContext.FOLDER_SOFTWARE); | ||
| discoverUsedSoftware(installedSoftwareTools, ideasyProjectSoftware, projectName); |
There was a problem hiding this comment.
Skip pointless recursion here?
| discoverUsedSoftware(installedSoftwareTools, ideasyProjectSoftware, projectName); | |
| discoverUsedSoftware(installedSoftwareTools, ideasyProjectSoftware, projectName, 1); |
| for (InstalledSoftwareTool tool : installedSoftwareTools) { | ||
| for (InstalledSoftwareEdition edition : tool.getEditions()) { | ||
| for (InstalledSoftwareVersion version : edition.getVersions()) { | ||
| Path versionPath = version.getPath(); | ||
| if (referencedPath.equals(versionPath) || referencedPath.startsWith(versionPath)) { | ||
| version.addUsedBy(projectName); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
So for complexity we do this
- for every M projects
- for every N software folders in project
- for every O installed tools
- for every P installed editions per tool
- for every Q versions per tool-edition
Looks quite inefficient to me.
Our data structure with InstalledSoftwareTool, -Edition, and -Version could be smarter having an internal Map from the versionPath to the InstalledSoftwareVersion populated when the data gets added (what should always happen via methods to make it possible to keep states consistent rather than simply doing get«SomeCollection»().add(...).
Then you have O(1) here instead of O(O*P*Q).
| public void addUsedBy(String projectName) { | ||
|
|
||
| if (!this.usedBy.contains(projectName)) { | ||
| this.usedBy.add(projectName); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is also odd design. What you want to archive here is exactly provided by a Set instead of a List.
This not only allows you to skip the if but also to make it more efficient since adding or contains check on Set is highly efficient unlike contains on List.
BTW: I just looked again into story #1580 where I wrote:
a Set with the IDEasy projects using this tool+edition+version. If empty, then it can be deleted.
Actually this is never used so we could also entirely omit it and just have deleted initialized with true in constructor and setting it to false whenever we find a project using it.
However, I think I suggested this design on purpose so we can later reuse this data structure in the GUI and show the end user a graphical tree where he can even see which projects are using what, etc.
So just change to Set and keep it.
| step.run(() -> discoverAndReportUnusedSoftware(installedSoftwareTools), true); | ||
|
|
||
| if (hasSoftwareToDelete(installedSoftwareTools)) { | ||
| // Automatically confirmed in batch mode with the global force option. | ||
| this.context.askToContinue("Do you want to continue?"); | ||
| deleteUnusedSoftware(installedSoftwareTools); |
There was a problem hiding this comment.
Reading this gives me some puzzle in my head:
If there is a question Do you want to continue? where is the information logged so I can answer this question?
Fortunately hasSoftwareToDelete has no side effect and from the naming we can see it is done in discoverAndReportUnusedSoftware.
However, wouldn't it make more sense to move logSoftwareToBeDeleted(installedSoftwareTools) out of discoverAndReportUnusedSoftware renaming it to just discoverUnusedSoftware and then before the if explicitly invoke the logging/reporting method?
I know this is just nice to have but I would better understand such code then...
6 participants
This PR fixes #1953
This PR completes and supersedes #1957 after the original author left the team.
Implemented changes:
-f/--forcemode instead of introducing a cleanup-specific force option.--force-deletehelp entries.commandlet.cleanuppackage._ide/software, includingdefault,maven, and custom repository IDs.Contents/MacOS.software/<tool>software/extra/<tool>software/extra/<tool>/<name>Testing instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
Related
Checklist 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.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
internalChecklist 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