#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 31 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 30448595757Coverage increased (+0.2%) to 72.801%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions7 previously-covered lines in 1 file 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...
| private final Path path; | ||
|
|
||
| /** A flag indicating whether the tool is marked for deletion. */ | ||
| private boolean delete; |
There was a problem hiding this comment.
Seems some of my review feedback got lost (was maybe set to resolved by original author):
Please use OOP and avoid pointless redundancies.
We have these 3 classes that all should have a common abstract super-class.
Each of them has
name(simply generalize to justnameinstead oftoolName,editionName,versionName)pathdelete
Also include methods like getters with JavaDoc, setter, etc.
BTW: I also suggested a simplification to have delete flag only in CleanupIdeToolEditionVersion so the consistency of the state gets trivial while with the current design it is complex. E.g. assume we later reuse this in the GUI and the user may see this as a tree with checkboxes and wants to unselect something manually to prevent deletion and then the parent deletion flag would need to be updated.
Simply when you iterate over the structure to perform the actual deletion check if the parent directory (edition or tool) now is empty after deletion of children and then also delete it.
| List<CleanupIdeTool> installedCleanupIdeTools = discoverInstalledSoftware(); | ||
|
|
||
| // Scan for IDEasy projects | ||
| List<Path> ideasyProjects = this.context.getFileAccess().listChildren(this.context.getIdeRoot(), Files::isDirectory); |
There was a problem hiding this comment.
We have the same logic also in our GUI. It would make sense to add a method IdeContext.findProjects() for this.
And as a motivation why I always insist on creating reusable structures:
This implementation is incorrect. It will also include _ide as well as folders somehow created by the end-user that are not really IDEasy projects. When we create a method and once get it right, then we can reuse it everywhere. Even if there still is a bug, we can fix it in once place instead of multiple places because of copy&paste.
| if (ideasyProject.getFileName().toString().equals("_ide")) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This would then be part of the central method.
| Path softwareRepositoryPath = this.context.getSoftwareRepositoryPath(); | ||
| List<Path> repositoryFolders = this.context.getFileAccess().listChildren(softwareRepositoryPath, Files::isDirectory); | ||
| for (Path repositoryFolder : repositoryFolders) { | ||
| discoverInstalledSoftwareRepository(installedCleanupIdeTools, repositoryFolder); |
There was a problem hiding this comment.
This is correct for default and maven but not for custom-tools:
https://github.com/devonfw/IDEasy/blob/main/documentation/software.adoc#custom-tools
For the given example URL
https://some-file-server.company.com/projects/my-project
We would have some-file-server.company.com as repositoryFolder here, but the actual repo with tool/edition/version structure is in the sub-path projects/my-project.
Sorry for this extra complication. I also missed this point in the review of the old PR.
So IMHO we should instead explicitly go to subfolders ToolRepository.ID_DEFAULT and MvnRepository.ID as well as this.context.getCustomToolRepository().getId(). The latter ID may be custom and that folder will typically not exist. Anyhow code should always be robust and handle non existing directories even when expected (I could delete entire _ide/software folder and that should not break ide cleanup - actually ide update should automatically repair my project if I did such evil deletion manually).
| installedCleanupIdeTools.add(tool); | ||
| discoverInstalledEditions(toolFolder, tool); |
There was a problem hiding this comment.
Just as a thought, the methods could return a boolean, if some tool/edition/version was found that looks sane. Then you could skip adding if nothing sane was found like this:
boolean success = discoverInstalledEditions(toolFolder, tool);
if (success) {
installedCleanupIdeTools.add(tool);
}
I am just still thinking if we have a reliable way of detecting "sanity".
We could obviously check for existence of .ide.software.version what would be my preferred way, but assuming this file was deleted by accident (see also #2197) it would IMHO still make sense to delete the tool installation if unused.
Then I was thinking that version folder should have a name that is a valid version.
However, we have seen some tools violating our own rules for what we think is a valid version.
Also checking that there is at least one digit in the version is wrong since we have version latest.
Maybe checking for latest or "at least one digit" might make sense.
So far just my thoughts - we can still ignore this for now and consider as improvement after the merge.
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