updates based on code review

Signed-off-by: Jeromy Cannon <jeromy@swirldslabs.com>
hashgraph · Sep 25, 2023 · db5283d · db5283d
1 parent 72757ac
commit db5283d
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 35 deletions.
diff --git a/...client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java b/...client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java
@@ -45,12 +45,12 @@ public final class HelmExecutionBuilder {
     /**
      * The arguments to be passed to the helm command.
      */
-    private final Set<KeyValuePair<String, String>> arguments;
+    private final HashMap<String, String> arguments;
 
     /**
-     * The arguments of sets to be passed to the helm command.
+     * The list of key value pairs where the value is a list of values for the given key.
      */
-    private final Set<KeyValuePair<String, Set<String>>> argumentsSet;
+    private final List<KeyValuePair<String, List<String>>> keyListOfValuesPairList;
 
     /**
      * The flags to be passed to the helm command.
@@ -80,14 +80,14 @@ 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 HashSet<>();
-        this.argumentsSet = new HashSet<>();
+        this.arguments = new HashMap<>();
+        this.keyListOfValuesPairList = new ArrayList<>();
         this.positionals = new ArrayList<>();
         this.flags = new ArrayList<>();
         this.environmentVariables = new HashMap<>();
 
         String workingDirectoryString = System.getenv("PWD");
-        this.workingDirectory = workingDirectoryString == null
+        this.workingDirectory = (workingDirectoryString == null || workingDirectoryString.isBlank())
                 ? this.helmExecutable.getParent()
                 : new File(workingDirectoryString).toPath();
     }
@@ -115,22 +115,23 @@ public HelmExecutionBuilder subcommands(final String... commands) {
     public HelmExecutionBuilder argument(final String name, final String value) {
         Objects.requireNonNull(name, NAME_MUST_NOT_BE_NULL);
         Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
-        this.arguments.add(new KeyValuePair<>(name, value));
+        this.arguments.put(name, value);
         return this;
     }
 
     /**
-     * Adds an argument of sets to the helm command.
+     * Adds a key with a provided list of values to the helm command.  This is used for options that have can have
+     * multiple values for a single key.  (e.g. --set and --values)
      *
-     * @param name  the name of the argument.
-     * @param value the set of value arguments.
+     * @param name  the key for the key/value pair.
+     * @param value the list of values for the given key.
      * @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) {
+    public HelmExecutionBuilder keyListOfValuesPair(final String name, final List<String> value) {
         Objects.requireNonNull(name, NAME_MUST_NOT_BE_NULL);
         Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
-        this.argumentsSet.add(new KeyValuePair<>(name, value));
+        this.keyListOfValuesPairList.add(new KeyValuePair<>(name, value));
         return this;
     }
 
@@ -219,12 +220,12 @@ private String[] buildCommand() {
         command.addAll(subcommands);
         command.addAll(flags);
 
-        arguments.forEach(entry -> {
-            command.add(String.format("--%s", entry.key()));
-            command.add(entry.value());
+        arguments.forEach((key, value) -> {
+            command.add(String.format("--%s", key));
+            command.add(value);
         });
 
-        argumentsSet.forEach(entry -> entry.value().forEach(value -> {
+        keyListOfValuesPairList.forEach(entry -> entry.value().forEach(value -> {
             command.add(String.format("--%s", entry.key()));
             command.add(value);
         }));

diff --git a/...ent/src/main/java/com/hedera/fullstack/helm/client/model/install/InstallChartOptions.java b/...ent/src/main/java/com/hedera/fullstack/helm/client/model/install/InstallChartOptions.java
@@ -18,7 +18,7 @@
 
 import com.hedera.fullstack.helm.client.execution.HelmExecutionBuilder;
 import com.hedera.fullstack.helm.client.model.Options;
-import java.util.Set;
+import java.util.List;
 
 /**
  * The options to be supplied to the helm install command.
@@ -56,11 +56,11 @@ public record InstallChartOptions(
         boolean passCredentials,
         String password,
         String repo,
-        Set<String> set,
+        List<String> set,
         boolean skipCrds,
         String timeout,
         String username,
-        Set<String> values,
+        List<String> values,
         boolean verify,
         String version,
         boolean waitFor)
@@ -98,7 +98,7 @@ public void apply(final HelmExecutionBuilder builder) {
         }
 
         if (set() != null) {
-            builder.argumentSet("set", set());
+            builder.keyListOfValuesPair("set", set());
         }
 
         if (timeout() != null) {
@@ -110,7 +110,7 @@ public void apply(final HelmExecutionBuilder builder) {
         }
 
         if (values() != null) {
-            builder.argumentSet("values", values());
+            builder.keyListOfValuesPair("values", values());
         }
 
         if (version() != null) {

diff --git a/.../main/java/com/hedera/fullstack/helm/client/model/install/InstallChartOptionsBuilder.java b/.../main/java/com/hedera/fullstack/helm/client/model/install/InstallChartOptionsBuilder.java
@@ -16,7 +16,7 @@
 
 package com.hedera.fullstack.helm.client.model.install;
 
-import java.util.Set;
+import java.util.List;
 
 /**
  * The builder for the {@link InstallChartOptions}.
@@ -31,11 +31,11 @@ public final class InstallChartOptionsBuilder {
     private boolean passCredentials;
     private String password;
     private String repo;
-    private Set<String> set;
+    private List<String> set;
     private boolean skipCrds;
     private String timeout;
     private String username;
-    private Set<String> values;
+    private List<String> values;
     private boolean verify;
     private String version;
     private boolean waitFor;
@@ -158,7 +158,7 @@ public InstallChartOptionsBuilder repo(String repo) {
      * @param valueOverride set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)
      * @return the current InstallChartOptionsBuilder.
      */
-    public InstallChartOptionsBuilder set(Set<String> valueOverride) {
+    public InstallChartOptionsBuilder set(List<String> valueOverride) {
         this.set = valueOverride;
         return this;
     }
@@ -202,7 +202,7 @@ public InstallChartOptionsBuilder username(String username) {
      * @param values specify values in a YAML file or a URL (can specify multiple).
      * @return the current InstallChartOptionsBuilder.
      */
-    public InstallChartOptionsBuilder values(Set<String> values) {
+    public InstallChartOptionsBuilder values(List<String> values) {
         this.values = values;
         return this;
     }

diff --git a/...c/test/java/com/hedera/fullstack/helm/client/test/execution/HelmExecutionBuilderTest.java b/...c/test/java/com/hedera/fullstack/helm/client/test/execution/HelmExecutionBuilderTest.java
@@ -25,14 +25,14 @@
 
 class HelmExecutionBuilderTest {
     @Test
-    @DisplayName("Test argumentSet null checks")
-    void testArgumentSetNullChecks() {
+    @DisplayName("Test keyListOfValuesPair null checks")
+    void testKeyListOfValuesPairNullChecks() {
         HelmExecutionBuilder builder = new HelmExecutionBuilder(new File(".").toPath());
         assertThrows(NullPointerException.class, () -> {
-            builder.argumentSet(null, null);
+            builder.keyListOfValuesPair(null, null);
         });
         assertThrows(NullPointerException.class, () -> {
-            builder.argumentSet("test string", null);
+            builder.keyListOfValuesPair("test string", null);
         });
     }
 

diff --git a/...test/java/com/hedera/fullstack/helm/client/test/model/InstallChartOptionsBuilderTest.java b/...test/java/com/hedera/fullstack/helm/client/test/model/InstallChartOptionsBuilderTest.java
@@ -17,14 +17,14 @@
 package com.hedera.fullstack.helm.client.test.model;
 
 import static org.junit.jupiter.api.Assertions.*;
-import static org.mockito.ArgumentMatchers.anySet;
+import static org.mockito.ArgumentMatchers.anyList;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
 import com.hedera.fullstack.helm.client.execution.HelmExecutionBuilder;
 import com.hedera.fullstack.helm.client.model.install.InstallChartOptions;
-import java.util.Set;
+import java.util.List;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -49,11 +49,11 @@ void testInstallChartOptionsBuilder() {
                 .passCredentials(true)
                 .password("password")
                 .repo("repo")
-                .set(Set.of("set", "livenessProbe.exec.command=[cat,docroot/CHANGELOG.txt]"))
+                .set(List.of("set", "livenessProbe.exec.command=[cat,docroot/CHANGELOG.txt]"))
                 .skipCrds(true)
                 .timeout("timeout")
                 .username("username")
-                .values(Set.of("values1", "values2"))
+                .values(List.of("values1", "values2"))
                 .verify(true)
                 .version("version")
                 .waitFor(true)
@@ -81,6 +81,6 @@ void testInstallChartOptionsBuilder() {
 
         options.apply(builderMock);
 
-        verify(builderMock, times(2)).argumentSet(anyString(), anySet());
+        verify(builderMock, times(2)).keyListOfValuesPair(anyString(), anyList());
     }
 }