#1937: link content from settings into workspace(s)#1983
Conversation
…(initial implementation)
Coverage Report for CI Build 27932932623Coverage increased (+0.003%) to 71.384%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions33 previously-covered lines in 3 files lost coverage.
Coverage Stats💛 - Coveralls |
|
Note: |
f7ec9f6 to
cb85dbe
Compare
hohwille
left a comment
There was a problem hiding this comment.
@tineff96 thanks for your PR. You found the right spots and your changes to config and properties seem perfect. 👍
I added some review comment to better reuse existing code avoiding redundancies.
Since you add a new feature, could you also add a JUnit test method covering it?
|
I can verify, that all tests including the new one are running successfully. The manual test also worked as described. The proposed changes look good so far, and I couldn't find anything to complain about. This PR can move on. |
hohwille
left a comment
There was a problem hiding this comment.
@tineff96 thanks for your PR. You implemented the story properly and added a JUnit to cover the feature. Nice job 👍
I added some comments for further improvements.
Nothing is wrong with your code, I am just suggesting to solve few things slightly more elegant.
| importRepository(config, repositoryPath, config.id()); | ||
| } | ||
| } else { | ||
| } else if (!config.isVirtualSettingsRepository()) { |
There was a problem hiding this comment.
IMHO this change is kind of pointless.
If we have a virtual settings repository then we can never enter the above if and then createRepository will always be true but since firstRepository will always be set nothing will be actually done here.
Therefore at this point this condition will always be true.
Nothing really wrong but confusing me if the condition is pointless.
| } | ||
|
|
||
| public boolean isVirtualSettingsRepository() { | ||
| return IdeContext.SETTINGS_REPOSITORY_KEYWORD.equals(this.id) |
There was a problem hiding this comment.
Actually the constant SETTINGS_REPOSITORY_KEYWORD was introduced as a duplicate to the already existing constant FOLDER_SETTINGS.
IMHO we should remove the duplicated constant, but that was not added by you...
| return IdeContext.SETTINGS_REPOSITORY_KEYWORD.equals(this.id) | |
| return IdeContext.FOLDER_SETTINGS.equals(this.id) |
| private static final String PROPERTY_LINK = "link"; | ||
| private static final String PROPERTY_LINK_TARGET = "link (=<target>)"; | ||
| private static final String PROPERTY_ECLIPSE = "eclipse"; | ||
| private static final String SETTINGS_PROPERTIES = "settings.properties"; |
There was a problem hiding this comment.
actually nothing wrong here, but it would be more elegant to move this code:
String filename = filePath.getFileName().toString();
final String id;
if (filename.endsWith(IdeContext.EXT_PROPERTIES)) {
id = filename.substring(0, filename.length() - IdeContext.EXT_PROPERTIES.length());
} else {
id = filename;
}
to a method String getId() in this class.
Then you do not need this constant and can implement isSettingsProperties() as:
return IdeContext.FOLDER_SETTINGS.equals(getId());
This PR fixes #1937
Implemented virtual settings repository feature. A
settings.propertieswithoutgit_urlnow indicates a virtual repository that reuses the already cloned$IDE_HOME/settingsfolder. Sub-folders fromsettingscan be linked intoworkspaces via the
linkproperty, e.g.:This avoids the need to create and maintain separate git repositories for
shared configuration content.
Implemented changes:
isVirtualSettingsRepository()toRepositoryConfig— returnstruewhen
id=settingsand nogit_urlis setcreateSettingsRepositoryLinks()toRepositoryCommandlet— iteratesover all configured workspaces and creates symlinks
linkTargetExists()toRepositoryCommandlet— validates link targetexists before creating symlink, logs warning and skips if not
git_urlvalidation inRepositoryPropertiesfor virtual settingsrepositories
testSetupVirtualSettingsRepository()Testing instructions
Automated test
A JUnit test was added for this feature and can be executed with:
The test verifies that a virtual settings repository without
git_urlcan create configured links from the settings directory into the workspace.Manual test
Create a subfolder in
$IDE_HOME/settings/, e.g.ai/, with any file inside.Create
$IDE_HOME/settings/repositories/settings.propertieswith content:Note: no
git_urlproperty is required. This is intentional for a virtual settings repository.Verify that
.githubis a link pointing to$IDE_HOME/settings/ai/in every workspace under$IDE_HOME/workspaces/.Run
ide repository setupa second time. It should complete without errors.Change the link target to a non-existing folder, e.g.:
Run again. It should log that the target does not exist and skip the link without crashing.
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