#1695: Clone settings to temporary directory, analyse, and then move - #2335
#1695: Clone settings to temporary directory, analyse, and then move#2335laim2003 wants to merge 18 commits into
Conversation
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
…itory. Signed-off-by: laim2003 <luk.faber@gmx.de>
…ir, verifies its health and then moves it to the IDE_HOME Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
…mpdir-verification' into feature/devonfw#1695-settings-tempdir-verification
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
…mpdir-verification' into feature/devonfw#1695-settings-tempdir-verification
Coverage Report for CI Build 32034542201Warning No base build found for commit Coverage: 72.824%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats💛 - Coveralls |
Signed-off-by: laim2003 <luk.faber@gmx.de>
|
|
||
| gitContext.pullOrClone(gitUrl, tempProjectPath); | ||
|
|
||
| checkIntegrityAndMove(tempProjectPath, gitUrl.getProjectName()); |
There was a problem hiding this comment.
Maybe it makes sense that the same way we use a new step for cloning the settings repo in general, in order to create a better user experience, we should also use a new step for the integrity check here? So it shows up as a separate step in the CLI? Just a thought.
hohwille
left a comment
There was a problem hiding this comment.
@laim2003 thanks for taking over and bringing this on the right track. 👍
I already left some comments to give early feedback since this story was already in end-review at least twice and we are turning in too many circles.
Please also note that if ide create fails then also no project shall even be created what is currently not the case.
My idea for this was that the update of the settings would be changed such that it returns a more complex result object that allows us to represent the following states:
- settings were already present and are valid (and maybe have been successfully updated)
- settings were cloned to temporary directory and are valid
- in all cases the information what type of repository we have (as
RepositoryType).
Other cases currently lead to an exception but could also be represented in that state if we prefer to take control of throwing exception outside.
Then the operation to move a potential temporary settings or "code with settings" repo to the right place can be in a separate method.
This allows the update flow to call both methods sequentially but the create flow to call the first one initially, then if that succeeded actually create the project and populate it, updating the context and only then call the 2nd method to move the repo into the right place.
Does that make sense or do you have a better idea?
| return; | ||
| } | ||
| this.context.newStep(getStepMessage()).run(() -> updateSettingsInStep(codeRepository)); | ||
| this.context.newStep(getStepMessage()).run(() -> updateSettingsInStep(codeRepository), true); |
There was a problem hiding this comment.
Do we really want to rethrow here?
This will make the process fail if any kind of exception is happening in updateSettingsInStep like e.g. "git pull" fails due to error of custom git server.
Currently I can still continue installing software updates then.
This will change now.
IMHO you want to archive that the further processing stops and fails if specific guardrails did not meet like the settings repo could not be cloned or it is not containing any settings.
Thinking this over and digging into step.run implementation, I would suggest the following:
- Currently we either handle or re-throw all exceptions there.
- I would suggest to add a method
CliException.isForceRethrowInStep()that by default returnsfalse. - Extend implementation of
step.runto also consider this method so we always re-throw in case that method returnstrue. - Create a
CliRethrowException(orCliFatalException) that overrides this new method returningtrue. - Explicitly throw this new exception in
updateSettingsInStepif no valid settings could be established.
BTW: We could even consider (in a separate PR if CliAbortException should also cause this effect (of isForceRethrowInStep()).
| */ | ||
| private void pullAndCheckIntegrity(GitUrl gitUrl) { | ||
| GitContext gitContext = this.context.getGitContext(); | ||
| Path tempProjectPath = this.context.getTempPath().resolve(IdeContext.FOLDER_PROJECTS).resolve(this.context.getProjectName()); |
There was a problem hiding this comment.
It would be safer to use FileAccess.createTempDir that guarantees us to produce a unique empty directory.
Otherwise you should ensure the directory is empty and not some leftover from a previous try is still present there (FileAccess.delete followed by FileAccess.mkdirs).
| import com.devonfw.tools.ide.context.IdeContext; | ||
| import com.devonfw.tools.ide.environment.EnvironmentVariables; | ||
|
|
||
| /// Utility class for IDEasy settings/code repositories |
There was a problem hiding this comment.
Use JavaDoc instead of inline comment.
| /// Utility class for IDEasy settings/code repositories | |
| /** Utility class for IDEasy settings/code repositories. */ |
| } else if (gitProjectName != null | ||
| && Files.exists(repositoryPath.resolve(IdeContext.FOLDER_SETTINGS)) | ||
| && getRepositoryType(repositoryPath.resolve(IdeContext.FOLDER_SETTINGS), gitProjectName) == RepositoryType.SETTINGS) { | ||
| return RepositoryType.CODE_SETTINGS_COMBINED; |
There was a problem hiding this comment.
Couldn't this unconstrainted recursion lead to accepting stuff like settings/settings/ide.properties?
I would at least ensure we do not run into recursion again:
| } else if (gitProjectName != null | |
| && Files.exists(repositoryPath.resolve(IdeContext.FOLDER_SETTINGS)) | |
| && getRepositoryType(repositoryPath.resolve(IdeContext.FOLDER_SETTINGS), gitProjectName) == RepositoryType.SETTINGS) { | |
| return RepositoryType.CODE_SETTINGS_COMBINED; | |
| } else if (gitProjectName != null | |
| && Files.exists(repositoryPath.resolve(IdeContext.FOLDER_SETTINGS)) | |
| && getRepositoryType(repositoryPath.resolve(IdeContext.FOLDER_SETTINGS), null) == RepositoryType.SETTINGS) { | |
| return RepositoryType.CODE_SETTINGS_COMBINED; |
| /// enum representation of a detected {@link RepositoryType} | ||
| public enum RepositoryType { | ||
| /// Git Repository is a code repository. | ||
| CODE, | ||
| /// Git Repository is a settings repository. | ||
| SETTINGS, | ||
| /// A combined code & settings repository contains both the settings-folder and the code within the workspace folder. | ||
| CODE_SETTINGS_COMBINED, | ||
| /// The type of the repository could not be determined. | ||
| UNKNOWN | ||
| } | ||
| } |
There was a problem hiding this comment.
- also here use JavaDoc instead of inline comments (BTW: inline comments in Java only have 2 slashes not 3).
- in general avoid nested types. It fully makes sense to have this enum as a top-level type in its own class. Esp. since it is used outside of this
RepositoryUtil.
This PR fixes #1695
Implemented changes:
ide create projectname <<git-url>>is pulled to a temporary directory. There, we check whether the repository is a valid settings or code-settings repository and if the check succeeds, we move it to the final location depending on the repo type (and symlink in case of acode-settingsrepo).--codeflag ofide createas we now automatically detect the type of the repository.Testing instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
Create commandlet
ide createusing the configurable running executions in intellij, so you dont accidentally test the create commandlet from the latest release, but the state from this PR. For the parameters: Pick a random project name. For thegit-urlparameter see below:IDE_HOME/workspaces/main/[repository name, in our case "code-settings-repo"]/settingsUpdate commandlet
In order to test the update commandlet, you need to create your own settings repository.
ide createcommandlet (again with the IntelliJ run configuration) and as thegit-urluse your newly created settings repo. It should succeed normally.NODE_VERSION=v18*tov23*, and commit the changes.ide eclipse.ide update(again, using the run configuration in intellij), it should succeed.ide updateagain. It should detect the empty folder and prompt you to input a new Git Url. You can use the same URL you used before. This step should also succeed.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.batand notfeature/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
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc