#1695: Clone settings to temporary directory, analyse, and then move#1878
#1695: Clone settings to temporary directory, analyse, and then move#1878areinicke wants to merge 21 commits into
Conversation
areinicke
added
enhancement
Coverage Report for CI Build 26281905478Coverage decreased (-0.1%) to 70.945%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions47 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
…n' of https://github.com/areinicke/IDEasy into feature/1695-clone-settings-to-temp-dir-for-verification
Co-authored-by: Robin Wenzel <robin@die-wenzels.de>
|
Thanks for the review. I have integrated the changes. |
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@areinicke thanks for your PR. Great that you implemented a test for it and finally started improving GitContextMock what I suggested long time ago when GitContextMock was supposed to be implemented. 👍
This is not a very easy task. Seems in such cases it makes sense to start with a design session before doing the complete PR implementation in the future...
However, should still not be a big change since most of your PR is already perfectly correct.
| // Check if instance of create commandlet. Only then will we analyze the project | ||
| if (this instanceof CreateCommandlet) { | ||
| analyzeProject(); | ||
| } |
There was a problem hiding this comment.
This design can be improved.
Option 1: you assume that you only need to analyse this when a project is initially created but not when updated.
Then simply override updateSettings() in CreateCommandlet and call analyseProject() after super.updateSettings(). Then only CreateCommandlet knows of analyseProject() (SoC).
Option 2: you need to analyse whenever you need to clone the settings.
Then this code is wrong since you skip this in case of UpdateCommandlet.
In such case you would need to do this after the cloning of the settings.
IMHO the option to ask for the settings in case of ide update is a kind of auto-repair feature of IDEasy.
Therefore ide update should behave also in the same way that it detects if we cloned an regular settings repo or a code repo with settings included.
But to get all this clean, we might need to revisit the design how create and update try to reuse the existing code/methods from AbstractUpdateCommandlet.
IMHO it would be better to do this on create:
- prepare a temp folder
- trigger the reusable functionality that clones the settings into a given project folder where here we provide the temporary folder. If it detected a code repo, it can move the settings to main workspace and proper folder name from the GitUrl and create the symlink to preserve a proper
settingsfolder in the projects root dir. - if that fails (neither regular settings repo nor code repo with settings found) we cleanup and abort.
- if that succeeded we can move the temp folder to the real project folder and continue with the regular logic where we re-initialize the IdeContext on the real project and do the full create/update process.
In case of ide update we can keep everything as is and reuse the same functionality described above in the 2nd point.
| // Repository seems to be invalid. Clean up temporary location and return error | ||
| fileAccess.delete(this.context.getIdeHome()); | ||
| throw new CliException("This repository does not include an " + EnvironmentVariables.DEFAULT_PROPERTIES + " or " + EnvironmentVariables.LEGACY_PROPERTIES + " file at the top level or a settings folder with such a file. " | ||
| + "The repository does not seem to be a valid IDEasy repository. Please verify the repository and try again."); |
There was a problem hiding this comment.
dont delete the entire project.
You can either delete the settings here or IMHO even better would be to just return failure (e.g. false while otherwise true is returned for success) and let the calling logic handle the cleanup so in case of ide update we can just delete the settings and in case of ide create we can delete the temp project.
| } | ||
| String userPromt = "Repository URL [" + IdeContext.DEFAULT_SETTINGS_REPO_URL + "]:"; | ||
| String defaultUrl = IdeContext.DEFAULT_SETTINGS_REPO_URL; | ||
| LOG.info(MESSAGE_SETTINGS_REPO_URL, this.context.getSettingsPath()); |
There was a problem hiding this comment.
fine but you then also need to include an update to the documentation that otherwise gets inconsistent:
https://github.com/devonfw/IDEasy/blob/main/documentation/settings.adoc#code-repository
5 participants
This PR fixes #1695
Implemented changes:
$IDE_ROOT/_ide/tmp/projects/<project name>) where it is analyzed for validity. We check whether an ide.properties file exist either at the top level or within a settings folder at the top level. Only if this passes, do we create a new project at$IDE_ROOT/<project name>and move the files from the temporary location to the final location. The temporary folder is fully deleted after this process.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
internal#Note: In some places, I used
getFileAccess()to copy and subsequently delete files. I am aware that there is a move function. However, this was always failing for some reason