feat: enhance Helm Install to support multiple set and values parameters #367

jeromy-cannon · 2023-09-21T19:28:50Z

Description

This pull request changes the following:

enhance Helm Install to support multiple set parameters
enhance Helm Install to support multiple values parameters
downgrade to traditional SLF4J methods to support Gradle's SLF4J 1.7 runtime
improve HelmExecution.responseAs() HelmExecutionException
improved HelmClientTest to reduce helm client commands and remove inconsistent test case failures
improve test coverage for ChartInstallRequest.apply()

PR Walkthrough (13 minutes): https://www.loom.com/share/bff374ec190a44c3aadac2f787bf13f7?sid=7e32aad3-abde-4f7b-90d6-4ba22485edd7

Related Issues

github-actions · 2023-09-21T19:47:18Z

Unit Test Results

17 files +1 17 suites +1 39s ⏱️ -1s
91 tests +6 88 ✔️ +6 3 💤 ±0 0 ❌ ±0
92 runs +6 89 ✔️ +6 3 💤 ±0 0 ❌ ±0

Results for commit 7c5b663. ± Comparison against base commit e91f465.

♻️ This comment has been updated with latest results.

#366 Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

nathanklick · 2023-09-25T13:57:36Z

...lm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java

@@ -72,11 +80,16 @@ public final class HelmExecutionBuilder {
    public HelmExecutionBuilder(final Path helmExecutable) {
        this.helmExecutable = Objects.requireNonNull(helmExecutable, "helmExecutable must not be null");
        this.subcommands = new ArrayList<>();
-        this.arguments = new HashMap<>();
+        this.arguments = new HashSet<>();
+        this.argumentsSet = new HashSet<>();


Nit: naming this is hard but it would be good to change this name to something not containing arguments.

yes, not my favorite. I tried to give something better. take a look and let me know if that is good.

updated a second time to optionsWithMultipleValues() which I like better

nathanklick · 2023-09-25T13:57:41Z

...lm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java

@@ -72,11 +80,16 @@ public final class HelmExecutionBuilder {
    public HelmExecutionBuilder(final Path helmExecutable) {
        this.helmExecutable = Objects.requireNonNull(helmExecutable, "helmExecutable must not be null");
        this.subcommands = new ArrayList<>();
-        this.arguments = new HashMap<>();
+        this.arguments = new HashSet<>();
+        this.argumentsSet = new HashSet<>();


Nit: naming this is hard but it would be good to change this name to something not containing arguments.

yes, not my favorite. I tried to give something better. take a look and let me know if that is good.

updated a second time to optionsWithMultipleValues() which I like better

nathanklick · 2023-09-25T13:59:36Z

...lm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java

@@ -72,11 +80,16 @@ public final class HelmExecutionBuilder {
    public HelmExecutionBuilder(final Path helmExecutable) {
        this.helmExecutable = Objects.requireNonNull(helmExecutable, "helmExecutable must not be null");
        this.subcommands = new ArrayList<>();
-        this.arguments = new HashMap<>();
+        this.arguments = new HashSet<>();


This needs to be a HashMap since some arguments may be repeatef and it is okay to do so.

nathanklick · 2023-09-25T14:00:12Z

...lm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java

@@ -72,11 +80,16 @@ public final class HelmExecutionBuilder {
    public HelmExecutionBuilder(final Path helmExecutable) {
        this.helmExecutable = Objects.requireNonNull(helmExecutable, "helmExecutable must not be null");
        this.subcommands = new ArrayList<>();
-        this.arguments = new HashMap<>();
+        this.arguments = new HashSet<>();
+        this.argumentsSet = new HashSet<>();


This should most likely be a list because deduplicating on key alone is going to be a problem.

nathanklick · 2023-09-25T14:01:21Z

...lm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java

-        this.workingDirectory = this.helmExecutable.getParent();
+
+        String workingDirectoryString = System.getenv("PWD");
+        this.workingDirectory = workingDirectoryString == null


Suggested change

this.workingDirectory = workingDirectoryString == null

this.workingDirectory = (workingDirectoryString == null || workingDirectoryString.isBlank())

nathanklick · 2023-09-25T14:02:32Z

...lm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java

+     * @return this builder.
+     * @throws NullPointerException if either {@code name} or {@code value} is {@code null}.
+     */
+    public HelmExecutionBuilder argumentSet(final String name, final Set<String> value) {


Nit: naming is hard but I think we should adjust this.

yes, not my favorite. I tried to give something better. take a look and let me know if that is good.

updated a second time to optionsWithMultipleValues() which I like better

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

sonarcloud · 2023-10-02T16:31:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

70.3% Coverage
0.0% Duplication

leninmehedy · 2023-10-04T04:26:40Z

...lm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java

+    /**
+     * The list of options and a list of their one or more values.
+     */
+    private final List<KeyValuePair<String, List<String>>> optionsWithMultipleValues;


Instead of naming it optionWithMultipleValues, flagWithMultipleValues would have been a bit more clearer.

I had to read through the code and also check helm install --help to understand that this variable adds support for all helm CLI flags that support multiple values.

my only concern about using 'flag', is that it is used in another location to represent the options that have no values. Whereas arguments is used for options that have values. But, Nathan challenged me to find one that didn't use the word 'argument'.

I'm going to move forward with this PR. But, if you still have feelings with it I can make changes in a future PR.

## [0.12.0](v0.11.0...v0.12.0) (2023-11-03) ### Features * add a Gradle task for downloading executable for Kubernetes Kind ([#425](#425)) ([aaa9660](aaa9660)) * add fullstack-gradle-plugin project to GitHub workflow ([#421](#421)) ([7b717ab](7b717ab)) * add Gateway API route for Hedera Explorer ([#466](#466)) ([c53943c](c53943c)) * add Gradle build docker image tasks ([#408](#408)) ([338cf40](338cf40)) * add gradle Helm dependency update task ([#393](#393)) ([8e3ead5](8e3ead5)) * add gradle Helm release exists task ([#389](#389)) ([0b7ae17](0b7ae17)) * add gradle Helm test task ([#403](#403)) ([591cebb](591cebb)) * add helm chart tests for gateway api routes ([#345](#345)) ([3936a64](3936a64)) * add helm dependency update subcommand ([#377](#377)) ([2b3609d](2b3609d)) * add helm execution gradle task: HelmInstallChartTask ([#304](#304)) ([6a29222](6a29222)) * add helm execution gradle task: HelmUninstallChartTask ([#375](#375)) ([ba6cc63](ba6cc63)) * add helm list releases subcommand ([#380](#380)) ([53d092f](53d092f)) * add helm test subcommand with options ([#376](#376)) ([5d08a32](5d08a32)) * add ifExists option to Gradle Helm uninstall task ([#405](#405)) ([0726725](0726725)) * add minio operator to fullstack-cluster-setup chart ([#453](#453)) ([bf7f6ff](bf7f6ff)) * add prometheus operator to fullstack-cluster-setup ([#460](#460)) ([0313c3a](0313c3a)) * add skipIfExists option for Gradle Helm install task ([#406](#406)) ([ccfbabf](ccfbabf)) * add support for scheduling pods based on node labels, taints, and affinity ([#352](#352)) ([5dd625a](5dd625a)) * Adding fabric8 k8s library and helm client for use in Infra API ([#386](#386)) ([3049fe4](3049fe4)) * Adding topology model classes and junit annotation processing ([#383](#383)) ([9c6930e](9c6930e)) * apply nodeSelector and tolerations to all pods including proxies ([#384](#384)) ([40e737a](40e737a)) * **cli:** add traceId in CLI logs for easier debugging ([#449](#449)) ([a1693c7](a1693c7)) * **cli:** implement chart install, uninstall and upgrade commands ([#454](#454)) ([70fd199](70fd199)) * **cli:** implement cluster create and delete commands ([#446](#446)) ([78be823](78be823)) * **cli:** implement cluster setup command ([#452](#452)) ([89c2662](89c2662)) * **cli:** implement tests and dependency checks for init command ([#438](#438)) ([b69dd99](b69dd99)) * enhance Helm Install to support multiple set and values parameters ([#367](#367)) ([05444df](05444df)) * implement fullstack-cluster-setup chart for shared resources ([#363](#363)) ([e91f465](e91f465)) * parameterize the helm chart namespace value ([#351](#351)) ([6d1c0a5](6d1c0a5)) * scaffold fsnetman CLI with ES6 support ([#424](#424)) ([65e06d9](65e06d9)) * upgrade to gradle 8.4 ([#404](#404)) ([69e769d](69e769d)) * upgrade to org.gradlex:java-module-dependencies:1.4.1 ([#365](#365)) ([ba7c32b](ba7c32b)) ### Bug Fixes * avoid hardcoding mirror-node-explorer rest API in values.yaml ([#413](#413)) ([0fe58c7](0fe58c7)) * **cli:** encapsulate helm and kubectl commands and code cleanup ([#476](#476)) ([1b8c499](1b8c499)) * **cli:** encapsulate Kind commands in a separate wrapper class ([#468](#468)) ([6dc6025](6dc6025)) * store hedera-explorer chart as dependency until next official version is released ([#436](#436)) ([201bda5](201bda5)) * support deploying the chart with custom release name ([#412](#412)) ([619b42b](619b42b)) * uninstall envoy-gateway should also remove gateway-system namespace that it created ([#350](#350)) ([8a2f6e1](8a2f6e1)) * update KeyValuePair.equals to only compare key (not value) ([#347](#347)) ([28222e0](28222e0))

swirlds-automation · 2023-11-03T17:30:02Z

🎉 This PR is included in version 0.12.0 🎉

The release is available on:

GitHub release
npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

jeromy-cannon self-assigned this Sep 21, 2023

jeromy-cannon changed the title ~~feat: Enhance Helm Install to support multiple set and values parameters~~ feat: enhance Helm Install to support multiple set and values parameters Sep 22, 2023

jeromy-cannon added 2 commits September 22, 2023 18:57

GMD: Enhance Helm Install to support multiple set and values parameters

c9c45f9

#366 Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

only %n for string.format

25ca488

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

jeromy-cannon force-pushed the 00366-helm-install-enhancements branch from 4aa8ba9 to 25ca488 Compare September 22, 2023 17:57

jeromy-cannon added 2 commits September 22, 2023 19:55

spotlessApply

622fd31

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

coverage for ChartInstallRequest.apply()

d1b70f8

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

jeromy-cannon requested a review from nathanklick September 22, 2023 19:16

jeromy-cannon marked this pull request as ready for review September 22, 2023 19:16

jeromy-cannon requested review from leninmehedy and deepak-swirlds September 22, 2023 19:17

sonar changes

72757ac

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

nathanklick reviewed Sep 25, 2023

View reviewed changes

updates based on code review

db5283d

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

jeromy-cannon marked this pull request as draft September 25, 2023 20:57

jeromy-cannon marked this pull request as ready for review September 26, 2023 08:42

better rename for options with multiple values

7c5b663

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>

leninmehedy reviewed Oct 4, 2023

View reviewed changes

leninmehedy approved these changes Oct 4, 2023

View reviewed changes

jeromy-cannon merged commit 05444df into main Oct 4, 2023
10 checks passed

jeromy-cannon deleted the 00366-helm-install-enhancements branch October 4, 2023 12:59

swirlds-automation added the released label Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enhance Helm Install to support multiple set and values parameters #367

feat: enhance Helm Install to support multiple set and values parameters #367

jeromy-cannon commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited

Loading

nathanklick Sep 25, 2023

jeromy-cannon Sep 25, 2023

jeromy-cannon Oct 2, 2023

nathanklick Sep 25, 2023

jeromy-cannon Sep 25, 2023

jeromy-cannon Oct 2, 2023

nathanklick Sep 25, 2023

jeromy-cannon Sep 25, 2023

nathanklick Sep 25, 2023

jeromy-cannon Sep 25, 2023

nathanklick Sep 25, 2023

jeromy-cannon Sep 25, 2023

nathanklick Sep 25, 2023

jeromy-cannon Sep 25, 2023

jeromy-cannon Oct 2, 2023

sonarcloud bot commented Oct 2, 2023

leninmehedy Oct 4, 2023 •

edited

Loading

jeromy-cannon Oct 4, 2023

swirlds-automation commented Nov 3, 2023

	this.workingDirectory = workingDirectoryString == null
	this.workingDirectory = (workingDirectoryString == null \|\| workingDirectoryString.isBlank())

feat: enhance Helm Install to support multiple set and values parameters #367

feat: enhance Helm Install to support multiple set and values parameters #367

Conversation

jeromy-cannon commented Sep 21, 2023 • edited Loading

Description

Related Issues

github-actions bot commented Sep 21, 2023 • edited Loading

Unit Test Results

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

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

sonarcloud bot commented Oct 2, 2023

leninmehedy Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swirlds-automation commented Nov 3, 2023

jeromy-cannon commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited

Loading

leninmehedy Oct 4, 2023 •

edited

Loading