#759: upgrade settings commandlet #820

leonrohne27 · 2024-11-28T19:12:34Z

Fixes: #759

Implements:

all devon.properties are renamed to ide.properties. In case both files exists in the same directory, devon.properties gets merged into ide.properties. ide.properties has priority here.
duplicated variables in the same ide.properties file are being removed
If variable DEVON_IDE_CUSTOM_TOOLS exists, it is converted into a custom-tools.json file in settings
if settings/devon folder exists, it is renamed to settings/templates, if settings/projects folder exists, it is renamed to settings/repositories
recursively traverses all files in settings/workspace and settings/*/workspace and replacse all known IdeVariables from CURLY into ANGLED syntax and legacy variable names are replaced with their new names.
for all .xml files that do not contain the XML namespace of our merger, we print a warning with the filepath saying that this file is most-likely outdated and print a link to a documentation page about this settings migration feature
documentation page with small instructions for those who migrate from devonfw-ide to IDEasy
if a replacement-patterns.properties is found in a workspace folder a warning will be logged if the file is not empty and the file will be deleted.
if settings/ide.properties does not contain IDE_VARIABLE_SYNTAX_LEGACY_SUPPORT_ENABLED=false it will be added.

Added the command cd to the shell commandlet that allows the user to switch the directory inside the ide shell

…dlet

…dlet.java Co-authored-by: jan-vcapgemini <[email protected]>

added current working directory path to prompt name improved existing directory check adjusted exit codes

removed whitespace from ide prompt

Changed variable name from file_path to filePath

Changed checkForXMLNameSpace() to checkForXmlNamespace()

cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeSettingsCommandlet.java

hohwille · 2024-11-29T11:24:08Z

cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeSettingsCommandlet.java

+      try {
+        List<String> readLines = Files.readAllLines(filePath);
+        String[] split;
+        for (String line : readLines) {
+          if (!line.contains("#") && !line.isEmpty()) {
+            if (line.contains("DEVON_IDE_CUSTOM_TOOLS")) {
+              createCustomToolsJson(line);
+              continue;
+            }
+            if (line.contains("DEVON_")) {
+              line = line.replace("DEVON_", "");
+            }
+            split = line.split("[ =]");
+            if (split.length == 3) {
+              devonProperties.put(split[1], new String[] { split[0], split[2] });
+            }
+            if (split.length == 2) {
+              devonProperties.put(split[0], split[1]);
+            }
+          }
+        }
+
+      } catch (IOException e) {
+        throw new RuntimeException(e);
+      }


You are duplicating the code to read a devon.properties and a ide.properties file.

IDEasy/cli/src/main/java/com/devonfw/tools/ide/environment/EnvironmentVariablesPropertiesFile.java

Lines 71 to 86 in 76124f7

try (BufferedReader reader = Files.newBufferedReader(this.propertiesFilePath)) {

String line;

do {

line = reader.readLine();

if (line != null) {

VariableLine variableLine = VariableLine.of(line, this.context, getSource());

String name = variableLine.getName();

if (name != null) {

VariableLine migratedVariableLine = migrateLine(variableLine, false);

if (migratedVariableLine == null) {

this.context.warning("Illegal variable definition: {}", variableLine);

continue;

}

String migratedName = migratedVariableLine.getName();

String migratedValue = migratedVariableLine.getValue();

boolean legacyVariable = IdeVariables.isLegacyVariable(name);

We should use our existing infrastructure to do that.

Also your implementation contains some problems. E.g. # can also be used inside a value (e.g. to separate branch of a git-url) and I guess you are trying to ignore comments here. Using String "fiddling" with methods such as contains is also dangerous since it will also match in unexpected cases. Surely not a very relevant scenario but rather an odd edge-case but in case a user has DISABLED_DEVON_IDE_CUSTOM_TOOLS=(...) in his config, this will cause undesired effects (and I have seen users who were not aware that you can outcomment a line by adding # at the beginning but were simply adding DISABLED_ as prefix). As you can see in the linked code we already have better infrastructure to parse a line and separate variable name and value. This should be reused.

After re-reading all the code of this method I understand this was an important blind spot.
You tried to reimplement a lot of things that have already been solved in our code but could not find this.
Therefore, you ran into all these challenges how to deal with comments, how to split into property name and value, etc.

hohwille · 2024-11-29T11:29:43Z

cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeSettingsCommandlet.java

+            if (set.getValue() instanceof String) {
+              Files.write(filePath, ("\n" + set.getKey().toString() + "=" + set.getValue().toString()).getBytes(), StandardOpenOption.APPEND);
+            }
+            if (set.getValue() instanceof String[]) {
+              String[] values = (String[]) set.getValue();


https://www.baeldung.com/java-pattern-matching-instanceof

Added BufferedReader which now only reads out the first 3 lines of all .xml files. Also, the BufferedReader now searches for a more consistent String

…hub.com/leonrohne27/IDEasy into implement/759-UpgradeSettingsCommandlet # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeSettingsCommandlet.java

hohwille · 2024-12-03T10:35:17Z

cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeSettingsCommandlet.java

+                      content = content.replace("{", "[");
+                      content = content.replace("}", "]");


this is wrong. It will destroy all JSON files.
We need to use VariableSyntax, like here:

IDEasy/cli/src/main/java/com/devonfw/tools/ide/environment/AbstractEnvironmentVariables.java

Lines 220 to 226 in 886cea3

Matcher matcher = syntax.getPattern().matcher(value);

if (!matcher.find()) {

return value;

}

StringBuilder sb = new StringBuilder(value.length() + EXTRA_CAPACITY);

do {

String variableName = syntax.getVariable(matcher);

# Conflicts: # CHANGELOG.adoc # cli/src/main/resources/nls/Help.properties # cli/src/main/resources/nls/Help_de.properties

added more tests added new CustomToolsJson class moved constants to IdeContext

changed return type of readCustomToolsFromJson to CustomToolsJson

hohwille

@jan-vcapgemini thanks for your nice improvements 👍

hohwille · 2024-12-19T08:37:02Z

cli/src/main/java/com/devonfw/tools/ide/context/IdeContext.java

+  String FILE_CUSTOM_TOOLS = "ide-custom-tools.json";
+
+  String DEVON_IDE_CUSTOM_TOOLS = "DEVON_IDE_CUSTOM_TOOLS";
+  String IDE_CUSTOM_TOOLS = "IDE_CUSTOM_TOOLS";


Please remove pointless constant.

Suggested change

String IDE_CUSTOM_TOOLS = "IDE_CUSTOM_TOOLS";

hohwille · 2024-12-19T08:37:51Z

cli/src/main/java/com/devonfw/tools/ide/context/IdeContext.java

+  /** The filename of the configuration file in the settings for this {@link CustomToolRepository}. */
+  String FILE_CUSTOM_TOOLS = "ide-custom-tools.json";
+
+  String DEVON_IDE_CUSTOM_TOOLS = "DEVON_IDE_CUSTOM_TOOLS";


Create in IdeVariables or give a name that indicates that this is a variable name?
Please also add JavaDoc.

hohwille · 2024-12-19T08:39:16Z

cli/src/main/java/com/devonfw/tools/ide/repo/CustomToolRepositoryImpl.java

+  public static CustomToolsJson readCustomToolsFromJson(IdeContext context, Path customToolsJsonPath) {
+    try (InputStream in = Files.newInputStream(customToolsJsonPath);
+        Reader reader = new InputStreamReader(in, StandardCharsets.UTF_8)) {
+
+      JsonReader jsonReader = Json.createReader(new BufferedReader(reader));
+      JsonStructure json = jsonReader.read();
+      JsonObject jsonRoot = requireObject(json);
+      String defaultUrl = getString(jsonRoot, "url", "");
+      JsonArray jsonTools = requireArray(jsonRoot.get("tools"));
+      List<CustomTool> customTools = new ArrayList<>();
+
+      for (JsonValue jsonTool : jsonTools) {
+        JsonObject jsonToolObject = requireObject(jsonTool);
+        String name = getString(jsonToolObject, "name");
+        String version = getString(jsonToolObject, "version");
+        String url = getString(jsonToolObject, "url", defaultUrl);
+        boolean osAgnostic = getBoolean(jsonToolObject, "os-agnostic", Boolean.FALSE);
+        boolean archAgnostic = getBoolean(jsonToolObject, "arch-agnostic", Boolean.TRUE);
+        if (url.isEmpty()) {
+          throw new IllegalStateException("Missing 'url' property for tool '" + name + "'!");
        }
-      } catch (Exception e) {
-        throw new IllegalStateException("Failed to read JSON from " + customToolsJson, e);
+        // TODO
+        String checksum = null;
+        CustomTool customTool = new CustomTool(name, VersionIdentifier.of(version), osAgnostic, archAgnostic, url,
+            checksum, context.getSystemInfo());
+        customTools.add(customTool);
      }
+      return new CustomToolsJson("tools", defaultUrl, customTools);
+    } catch (Exception e) {
+      throw new IllegalStateException("Failed to read JSON from " + customToolsJsonPath, e);
    }
-    return new CustomToolRepositoryImpl(context, tools);
  }


This method and logic belong to CustomToolsJson class. Furhter, we can use Jackson for the mapping instead of writing the mapping code manually. We can add an internal method for post-processing that will set the url in all CustomTool instances to the parent one from CustomToolsJson in case it is null. This method can then be called both from the JSON parsing as well as from the String parsing method.

hohwille · 2024-12-19T08:42:20Z

cli/src/test/resources/customtools/custom-tools.json

@@ -0,0 +1,18 @@
+{


Please rename this file to ide-custom-tools.json to keep our own contract.

hohwille · 2024-12-19T08:45:52Z

cli/src/test/java/com/devonfw/tools/ide/repo/CustomToolTest.java

+    context.setSystemInfo(systemInfo);
+    Path testPath = Path.of("src/test/resources/customtools");
+    // act
+    CustomToolsJson customToolsJson = CustomToolRepositoryImpl.readCustomToolsFromJson(context, testPath.resolve("custom-tools.json"));


Great that you added new test methods to increase coverage. 👍
As I already commented on the main code please follow SoC and put the code into the classes responsible for what is done by the according method.
Then do the same for JUnits:
If you test a class from src/main/java/my/package/MyClass.java then the unit tests shall be in src/test/java/my/package/MyClassTest.java.
Hence you should create CustomToolsJsonTest instead of extending this test.

hohwille · 2024-12-19T08:53:20Z

cli/src/test/java/com/devonfw/tools/ide/repo/CustomToolTest.java

+  public void testReadCustomToolsFromLegacyConfig() {
+    // arrange
+    IdeContext context = new IdeTestContext();
+    String legacyProperties = "DEVON_IDE_CUSTOM_TOOLS=(jboss-eap:7.1.4.GA:all:https://host.tld/projects/my-project firefox:70.0.1:all:https://host.tld/projects/my-project2)";


I already left review comments to Kian and Leon that the context of that method is kind of odd and also we need to reuse our existing infrastructure to parse ide.properties and devon.properties files.
Then we can just get the value of a variable and only need to pass that value to this method instead of fiddling the value from the variable inside a class that is not responsible for that and code gets more redundant.
BTW: We also have existing code to parse variable array syntax into a regular Java List. This is currently in VariableDefinitionStringList e.g. used by IdeVariables.IDE_TOOLS to parse things like IDE_TOOLS=(java mvn node npm) (see https://github.com/devonfw/IDEasy/blob/main/documentation/variables.adoc).
We could even extract that parsing code to VariableLine for easier reuse here.

From the test perspective I would then only pass the value and not the entire variable declaration line.

hohwille · 2024-12-19T08:55:06Z

...src/test/resources/ide-projects/upgrade-settings/_ide/urls/java/java/17.0.6/linux_x64.sha256

@@ -0,0 +1 @@
+c2de7dfbd9f8faaa21b4cdd8518f826dd558c9ab24a0616b3ed28437a674a97b


since when do we add checksums for test projects? Are they actually used? I can not see how this could be used since the java installation in our tests is not yet downloaded as archive but only copied as directory tree.
If these *.sha256 files are all not needed or used, please remove them.

… in workspace templates

replaced regex parsing with indexOf added JsonIgnores and JsonProperties to CustomTool converted version from versionIdentifier to String added Tool record to CustomToolsJson

moved fromString into VariableLine replaced parenthesis parsing with VariableLine.fromString

Kian and others added 30 commits October 2, 2024 16:17

devonfw#765: Fix typos in DoD asciidoc

7af9fc5

Merge branch 'main' of https://github.com/KianRolf/IDEasy

aeae1dd

Merge branch 'main' of https://github.com/KianRolf/IDEasy

e520d16

Merge branch 'main' of https://github.com/KianRolf/IDEasy

29403d3

Merge branch 'main' of https://github.com/KianRolf/IDEasy

5bff5eb

Merge branch 'main' of https://github.com/KianRolf/IDEasy

890c107

Merge branch 'main' of https://github.com/KianRolf/IDEasy

c55af62

devonfw#737: Added cd command to shell commandlet

3be67d0

Added the command cd to the shell commandlet that allows the user to switch the directory inside the ide shell

Merge branch 'main' into implement/737-add-cd-command-to-shell-comman…

60f2e8a

…dlet

Merge branch 'main' of https://github.com/KianRolf/IDEasy

5224bf9

Merge branch 'main' into implement/737-add-cd-command-to-shell-comman…

02cdeb0

…dlet

Merge branch 'main' of https://github.com/KianRolf/IDEasy

d925977

Merge branch 'main' into implement/737-add-cd-command-to-shell-comman…

2d41161

…dlet

Merge branch 'main' of https://github.com/KianRolf/IDEasy

4a8f60b

Merge branch 'main' of https://github.com/KianRolf/IDEasy

1a6cdb8

Merge branch 'main' into implement/737-add-cd-command-to-shell-comman…

bad672f

…dlet

Merge branch 'main' of https://github.com/KianRolf/IDEasy

f79c030

Merge branch 'main' of https://github.com/KianRolf/IDEasy

3d2b190

Merge branch 'main' of https://github.com/KianRolf/IDEasy

e1854bb

Merge branch 'main' into implement/737-add-cd-command-to-shell-comman…

b85ca18

…dlet

Update cli/src/main/java/com/devonfw/tools/ide/commandlet/ShellComman…

51dd0c2

…dlet.java Co-authored-by: jan-vcapgemini <[email protected]>

Update cli/src/main/java/com/devonfw/tools/ide/commandlet/ShellComman…

0bd754c

…dlet.java Co-authored-by: jan-vcapgemini <[email protected]>

Merge branch 'main' of https://github.com/KianRolf/IDEasy

0302acf

devonfw#737: optimized cd command

1af2948

added current working directory path to prompt name improved existing directory check adjusted exit codes

devonfw#737: optimized cd command

ec4d5f6

removed whitespace from ide prompt

devonfw#737: Added Changelog entry

06e9b8a

devonfw#759:

4602866

kian ist der best

7bb5674

devonfw#759: listchildrenrecursive

50b9fb2

devonfw#759: rename

07ae66e

leonrohne27 added 2 commits November 29, 2024 12:11

devonfw#759: file_path to filePath

d7f60da

Changed variable name from file_path to filePath

devonfw#759: Changed method name

6f3229e

Changed checkForXMLNameSpace() to checkForXmlNamespace()

hohwille reviewed Nov 29, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeSettingsCommandlet.java Outdated Show resolved Hide resolved

hohwille reviewed Nov 29, 2024

View reviewed changes

devonfw#759: changed checkForXmlNamespace() method

afaa4f7

Added BufferedReader which now only reads out the first 3 lines of all .xml files. Also, the BufferedReader now searches for a more consistent String

hohwille added this to the release:2024.12.001 milestone Nov 29, 2024

KianRolf and others added 5 commits November 29, 2024 12:53

Merge branch 'implement/759-UpgradeSettingsCommandlet' of https://git…

3322aa2

…hub.com/leonrohne27/IDEasy into implement/759-UpgradeSettingsCommandlet # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/commandlet/UpgradeSettingsCommandlet.java

devonfw#759: Test for commandlet

589eb25

devonfw#759: Javadoc

97cae94

devonfw#759: clean up

1801808

devonfw#759: changed structure for XmlNamespace() method

694a7b8

hohwille reviewed Dec 3, 2024

View reviewed changes

jan-vcapgemini assigned jan-vcapgemini and unassigned KianRolf and leonrohne27 Dec 4, 2024

hohwille modified the milestones: release:2024.12.001, release:2024.12.002 Dec 12, 2024

jan-vcapgemini and others added 5 commits December 16, 2024 11:04

Merge branch 'main' into implement/759-UpgradeSettingsCommandlet

84ed5a4

# Conflicts: # CHANGELOG.adoc # cli/src/main/resources/nls/Help.properties # cli/src/main/resources/nls/Help_de.properties

Merge branch 'main' into implement/759-UpgradeSettingsCommandlet

1383b7b

devonfw#759: optimized parsing of custom tools

b614c6d

added more tests added new CustomToolsJson class moved constants to IdeContext

devonfw#759: added missing changes

6408d55

changed return type of readCustomToolsFromJson to CustomToolsJson

devonfw#759: fixed test

1902729

hohwille reviewed Dec 19, 2024

View reviewed changes

devonfw#759: rewritten implementation to upgrade variable expressions…

3c85ec1

… in workspace templates

hohwille modified the milestones: release:2024.12.002, release:2025.01.001 Dec 20, 2024

jan-vcapgemini and others added 3 commits December 20, 2024 12:00

devonfw#759: implemented requested changes

7f2d622

replaced regex parsing with indexOf added JsonIgnores and JsonProperties to CustomTool converted version from versionIdentifier to String added Tool record to CustomToolsJson

devonfw#759: push changes of DirectoryMergerTest

c4f3aa5

devonfw#759: implemented requested changes

9960940

moved fromString into VariableLine replaced parenthesis parsing with VariableLine.fromString

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#759: upgrade settings commandlet #820

#759: upgrade settings commandlet #820

leonrohne27 commented Nov 28, 2024

hohwille Nov 29, 2024

hohwille Nov 29, 2024

hohwille Nov 29, 2024

hohwille Dec 3, 2024

hohwille left a comment

hohwille Dec 19, 2024

hohwille Dec 19, 2024

hohwille Dec 19, 2024

hohwille Dec 19, 2024

hohwille Dec 19, 2024

hohwille Dec 19, 2024

hohwille Dec 19, 2024

	try (BufferedReader reader = Files.newBufferedReader(this.propertiesFilePath)) {
	String line;
	do {
	line = reader.readLine();
	if (line != null) {
	VariableLine variableLine = VariableLine.of(line, this.context, getSource());
	String name = variableLine.getName();
	if (name != null) {
	VariableLine migratedVariableLine = migrateLine(variableLine, false);
	if (migratedVariableLine == null) {
	this.context.warning("Illegal variable definition: {}", variableLine);
	continue;
	}
	String migratedName = migratedVariableLine.getName();
	String migratedValue = migratedVariableLine.getValue();
	boolean legacyVariable = IdeVariables.isLegacyVariable(name);

		content = content.replace("{", "[");
		content = content.replace("}", "]");

	Matcher matcher = syntax.getPattern().matcher(value);
	if (!matcher.find()) {
	return value;
	}
	StringBuilder sb = new StringBuilder(value.length() + EXTRA_CAPACITY);
	do {
	String variableName = syntax.getVariable(matcher);

		@@ -0,0 +1 @@
		c2de7dfbd9f8faaa21b4cdd8518f826dd558c9ab24a0616b3ed28437a674a97b

#759: upgrade settings commandlet #820

Are you sure you want to change the base?

#759: upgrade settings commandlet #820

Conversation

leonrohne27 commented Nov 28, 2024

Fixes: #759

Implements:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hohwille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment