#317: added BuildCommandlet #340

jan-vcapgemini · 2024-05-17T13:50:32Z

Fixes: #317

Implements

added BuildCommandlet for mvn, gradle and npm
added BuildCommandletTests
added documentation for build and build tools (mvn, npm, yarn)
added TOOL_BUILD_OPTS environment variables and handling

added BuildCommandlet added BuildCommandletTest added BuildCommandletTest resources

set npm test to parameterized to enforce windows only

# Conflicts: # cli/src/main/resources/nls/Ide.properties # cli/src/main/resources/nls/Ide_de.properties

added missing val path property to nls files adjusted assertLogMessage (removed parameterized test)

coveralls · 2024-05-21T09:11:58Z

Pull Request Test Coverage Report for Build 9301625045

Details

0 of 0 changed or added relevant lines in 0 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.6%) to 60.342%

Files with Coverage Reduction	New Missed Lines	%
com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java	3	89.77%

Totals
Change from base Build 9275378992:	0.6%
Covered Lines:	4733
Relevant Lines:	7542

💛 - Coveralls

aBega2000

Thank you for this PR. Everything seems fine to me.

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

hohwille

@jan-vcapgemini thanks for this PR. You did a great job implementing this and especially testing this commandlet with all its scenarios. 👍

However, there is some concern with the CLI properties and related behavior that differs from devonfw-ide. Please have a look at my review comments.

changed PathProperty to multivalued StringPropertery adjusted tests

added build documentation added mvn documentation added npm documentation added yarn documentation

replaced devon with ide

added default build options environment variables and handling added gradle documentation added default options to documentations added test for default options

hohwille

@jan-vcapgemini Thanks for your update. You did some great improvements 👍
Probably for complex stories I should next time do the reverse-engineering myself and write everything explicitly as requirements and acceptance criteria into the story. Due to lack of time, I created all the commandlet stories via copy&paste with a simple link to the old code and expected the developer to do the reverse-engineering. As it turns out this seems to be more tricky than expected...
Please have a look at my review comments.

hohwille · 2024-06-13T09:21:00Z

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

+    Path buildPath = this.context.getCwd();
+    String[] defaultToolOptions = new String[0];
+
+    if (buildPath != null) {


so if buildPath (aka cwd) would be null then we do nothing?
IMHO this will never happen but if we want to handle this explicitly, I would rather do something like this:

if (buildPath == null) { throw new CliException("Missing current working directory!"); }

hohwille · 2024-06-13T09:23:16Z

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

+
+          defaultToolOptions = getDefaultToolOptions(NPM_BUILD_OPTS.getName());
+        }
+      }


again we have no else here. So what if neither pom.xml nor build.gradle nor package.json was found?
Then we silently do nothing?
Again I would recommend explicit error handling:

Suggested change

}

} else {

throw new CliException("Could not find build descriptor - no pom.xml, build.gradle, or package.json found!");

}

hohwille · 2024-06-13T09:27:32Z

cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java

@@ -49,6 +49,18 @@ public interface IdeVariables {

  /** {@link VariableDefinition} for {@link com.devonfw.tools.ide.context.IdeContext#getWorkspaceName() WORKSPACE}. */

+  /** {@link VariableDefinition} for default build options of mvn */
+  VariableDefinitionString MVN_BUILD_OPTS = new VariableDefinitionString("MVN_BUILD_OPTS", null, c -> "");


As pointed out: https://github.com/devonfw/ide/blob/cdf4af5de21489f3fdcbfdab41af858822e0d598/scripts/src/main/resources/scripts/command/mvn#L354

Suggested change

VariableDefinitionString MVN_BUILD_OPTS = new VariableDefinitionString("MVN_BUILD_OPTS", null, c -> "");

VariableDefinitionString MVN_BUILD_OPTS = new VariableDefinitionString("MVN_BUILD_OPTS", null, c -> "clean deploy");

hohwille · 2024-06-13T09:31:57Z

cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java

+  VariableDefinitionString MVN_BUILD_OPTS = new VariableDefinitionString("MVN_BUILD_OPTS", null, c -> "");
+
+  /** {@link VariableDefinition} for default build options of npm */
+  VariableDefinitionString NPM_BUILD_OPTS = new VariableDefinitionString("NPM_BUILD_OPTS", null, c -> "");


This one seems more tricky - see
https://github.com/devonfw/ide/blob/cdf4af5de21489f3fdcbfdab41af858822e0d598/scripts/src/main/resources/scripts/command/npm#L118-L124
and
https://github.com/devonfw/ide/blob/cdf4af5de21489f3fdcbfdab41af858822e0d598/scripts/src/main/resources/scripts/command/npm#L92-L98

Maybe we should address this one in a separate PR and merge as is? So far this would be my suggestion to make better progress... So either create an issue for this and link here as comment so I can resolve or already address in this PR adding even more new features and complexity.

hohwille · 2024-06-13T09:33:18Z

cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java

+  VariableDefinitionString NPM_BUILD_OPTS = new VariableDefinitionString("NPM_BUILD_OPTS", null, c -> "");
+
+  /** {@link VariableDefinition} for default build options of gradle */
+  VariableDefinitionString GRADLE_BUILD_OPTS = new VariableDefinitionString("GRADLE_BUILD_OPTS", null, c -> "");


Again see here: https://github.com/devonfw/ide/blob/cdf4af5de21489f3fdcbfdab41af858822e0d598/scripts/src/main/resources/scripts/command/gradle#L91

Suggested change

VariableDefinitionString GRADLE_BUILD_OPTS = new VariableDefinitionString("GRADLE_BUILD_OPTS", null, c -> "");

VariableDefinitionString GRADLE_BUILD_OPTS = new VariableDefinitionString("GRADLE_BUILD_OPTS", null, c -> "clean dist");

hohwille · 2024-06-13T09:34:55Z

cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java

+  VariableDefinitionString GRADLE_BUILD_OPTS = new VariableDefinitionString("GRADLE_BUILD_OPTS", null, c -> "");
+
+  /** {@link VariableDefinition} for default build options of yarn */
+  VariableDefinitionString YARN_BUILD_OPTS = new VariableDefinitionString("YARN_BUILD_OPTS", null, c -> "");


Same as for NPM:
https://github.com/devonfw/ide/blob/cdf4af5de21489f3fdcbfdab41af858822e0d598/scripts/src/main/resources/scripts/command/yarn#L88-L94

hohwille · 2024-06-13T09:38:39Z

cli/src/test/resources/ide-projects/build/project/settings/ide.properties

+NODE_VERSION=v18.19.1
+NPM_VERSION=9.9.2
+GRADLE_VERSION=8.7
+MVN_BUILD_OPTS=clean install


This is the default that is supposed to be build-in. Either remove or override with something else to test the overriding mechanism.

hohwille · 2024-06-13T09:45:28Z

documentation/build.adoc

@@ -0,0 +1,12 @@
+:toc:


As discussed in our meetings we do not want to come back to having help documentation on github in asciidoc.
This has many disadvantages (not available offline, esp. not accurate if the installed release differs from the latest documentation in development, no localization, not intuitive for CLI users, etc.).
Simply use your invest to put this into the CLI help output. Also please add the new variables to the variables.adoc. Then you can remove these 5 added adocs.

For this we already agreed in the team to add yet another help property for the detailed description of a commandlet.
So ide help «commandlet» will not only print cmd.build but also cmd.build.detail.
I can prepare the PR for this change.

Implemented:
#389

devonfw#317: added BuildCommandlet

ebc970a

added BuildCommandlet added BuildCommandletTest added BuildCommandletTest resources

jan-vcapgemini self-assigned this May 17, 2024

jan-vcapgemini added enhancement New feature or request npm node package manager mvn related to apache maven build tool gradle Gralde build tool commandlet labels May 17, 2024

jan-vcapgemini added 5 commits May 17, 2024 15:52

Merge branch 'main' into feature/317-build-commandlet

c5a3df1

devonfw#317: fixed npm test

99eb686

set npm test to parameterized to enforce windows only

Merge branch 'main' into feature/317-build-commandlet

fdf83cb

# Conflicts: # cli/src/main/resources/nls/Ide.properties # cli/src/main/resources/nls/Ide_de.properties

devonfw#317: fixed nls format

ea88658

devonfw#317: fixed tests

efc9081

added missing val path property to nls files adjusted assertLogMessage (removed parameterized test)

Merge branch 'main' into feature/317-build-commandlet

fbfec39

aBega2000 approved these changes May 23, 2024

View reviewed changes

hohwille reviewed May 24, 2024

View reviewed changes

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

hohwille reviewed May 24, 2024

View reviewed changes

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

hohwille requested changes May 24, 2024

View reviewed changes

jan-vcapgemini added 7 commits May 29, 2024 11:26

Merge branch 'main' into feature/317-build-commandlet

66dcb1a

devonfw#317: implemented requested changes

823b241

changed PathProperty to multivalued StringPropertery adjusted tests

devonfw#317: fixed npm test

ed33422

devonfw#317: adjusted help

f9cdca7

devonfw#317: added documentation files

976abb3

added build documentation added mvn documentation added npm documentation added yarn documentation

devonfw#317: adjusted documentation

72075c9

replaced devon with ide

devonfw#12: implemented requested changes

c129a41

added default build options environment variables and handling added gradle documentation added default options to documentations added test for default options

jan-vcapgemini requested a review from hohwille May 30, 2024 11:22

jan-vcapgemini assigned hohwille May 30, 2024

jan-vcapgemini added this to the release:2024.06.002 milestone May 30, 2024

hohwille approved these changes Jun 13, 2024

View reviewed changes

hohwille reviewed Jun 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#317: added BuildCommandlet #340

#317: added BuildCommandlet #340

jan-vcapgemini commented May 17, 2024 •

edited

coveralls commented May 21, 2024 •

edited

aBega2000 left a comment

hohwille left a comment •

edited

hohwille left a comment

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

hohwille Jun 13, 2024

-      }
+      } else {
+        throw new CliException("Could not find build descriptor - no pom.xml, build.gradle, or package.json found!");
+      }

	VariableDefinitionString MVN_BUILD_OPTS = new VariableDefinitionString("MVN_BUILD_OPTS", null, c -> "");
	VariableDefinitionString MVN_BUILD_OPTS = new VariableDefinitionString("MVN_BUILD_OPTS", null, c -> "clean deploy");

	VariableDefinitionString GRADLE_BUILD_OPTS = new VariableDefinitionString("GRADLE_BUILD_OPTS", null, c -> "");
	VariableDefinitionString GRADLE_BUILD_OPTS = new VariableDefinitionString("GRADLE_BUILD_OPTS", null, c -> "clean dist");

#317: added BuildCommandlet #340

Are you sure you want to change the base?

#317: added BuildCommandlet #340

Conversation

jan-vcapgemini commented May 17, 2024 • edited

coveralls commented May 21, 2024 • edited

Pull Request Test Coverage Report for Build 9301625045

Details

💛 - Coveralls

aBega2000 left a comment

Choose a reason for hiding this comment

hohwille left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-vcapgemini commented May 17, 2024 •

edited

coveralls commented May 21, 2024 •

edited

hohwille left a comment •

edited