Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Oct 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c9c45f9
GMD: Enhance Helm Install to support multiple set and values paramete…
jeromy-cannon Sep 21, 2023
25ca488
only %n for string.format
jeromy-cannon Sep 22, 2023
622fd31
spotlessApply
jeromy-cannon Sep 22, 2023
d1b70f8
coverage for ChartInstallRequest.apply()
jeromy-cannon Sep 22, 2023
72757ac
sonar changes
jeromy-cannon Sep 25, 2023
db5283d
updates based on code review
jeromy-cannon Sep 25, 2023
7c5b663
better rename for options with multiple values
jeromy-cannon Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 36 additions & 43 deletions ...k-helm-client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,30 +218,29 @@ public <T> T responseAs(final Class<T> responseClass, final Duration timeout) {
}

if (exitCode() != 0) {
throw new HelmExecutionException(exitCode());
throw new HelmExecutionException(
exitCode(),
StreamUtils.streamToString(suppressExceptions(this::standardOutput)),
StreamUtils.streamToString(suppressExceptions(this::standardError)));
}

final String standardOutput = StreamUtils.streamToString(suppressExceptions(this::standardOutput));
final String standardError = StreamUtils.streamToString(suppressExceptions(this::standardError));

LOGGER.atDebug()
.setMessage(
"ResponseAs exiting with exitCode: {}\n\tResponseClass: {}\n\tstandardOutput: {}\n\tstandardError: {}")
.addArgument(this::exitCode)
.addArgument(responseClass.getName())
.addArgument(standardOutput)
.addArgument(standardError)
.log();
LOGGER.debug(
"ResponseAs exiting with exitCode: {}TODO\n\tResponseClass: {}\n\tstandardOutput: {}\n\tstandardError: {}",
exitCode(),
responseClass.getName(),
standardOutput,
standardError);

try {
return OBJECT_MAPPER.readValue(standardOutput, responseClass);
} catch (final Exception e) {
LOGGER.atWarn()
.setMessage("ResponseAs failed to deserialize response into class: {}\n\tresponse: {}")
.addArgument(responseClass.getName())
.addArgument(standardOutput)
.setCause(e)
.log();
LOGGER.warn(
String.format(
"ResponseAs failed to deserialize response into class: %s%n\tresponse: %s",
responseClass.getName(), standardOutput),
e);

throw new HelmParserException(String.format(MSG_DESERIALIZATION_ERROR, responseClass.getName()), e);
}
Expand Down Expand Up @@ -291,28 +290,24 @@ public <T> List<T> responseAsList(final Class<T> responseClass, final Duration t
final String standardOutput = StreamUtils.streamToString(suppressExceptions(this::standardOutput));
final String standardError = StreamUtils.streamToString(suppressExceptions(this::standardError));

LOGGER.atDebug()
.setMessage(
"ResponseAsList exiting with exitCode: {}\n\tResponseClass: {}\n\tstandardOutput: {}\n\tstandardError: {}")
.addArgument(this::exitCode)
.addArgument(responseClass.getName())
.addArgument(standardOutput)
.addArgument(standardError)
.log();
LOGGER.debug(
"ResponseAsList exiting with exitCode: {}\n\tResponseClass: {}\n\tstandardOutput: {}\n\tstandardError: {}",
exitCode(),
responseClass.getName(),
standardOutput,
standardError);

try {
return OBJECT_MAPPER
.readerFor(responseClass)
.<T>readValues(standardOutput)
.readAll();
} catch (final Exception e) {
LOGGER.atWarn()
.setMessage(
"ResponseAsList failed to deserialize the output into a list of the specified class: {}\n\tresponse: {}")
.addArgument(responseClass.getName())
.addArgument(standardOutput)
.setCause(e)
.log();
LOGGER.warn(
String.format(
"ResponseAsList failed to deserialize the output into a list of the specified class: %s%n\tresponse: %s",
responseClass.getName(), standardOutput),
e);

throw new HelmParserException(String.format(MSG_LIST_DESERIALIZATION_ERROR, responseClass.getName()), e);
}
Expand Down Expand Up @@ -349,20 +344,18 @@ public void call(final Duration timeout) {
final String standardOutput = StreamUtils.streamToString(suppressExceptions(this::standardOutput));
final String standardError = StreamUtils.streamToString(suppressExceptions(this::standardError));

LOGGER.atDebug()
.setMessage("Call exiting with exitCode: {}\n\tstandardOutput: {}\n\tstandardError: {}")
.addArgument(this::exitCode)
.addArgument(standardOutput)
.addArgument(standardError)
.log();
LOGGER.debug(
"Call exiting with exitCode: {}\n\tstandardOutput: {}\n\tstandardError: {}",
exitCode(),
standardOutput,
standardError);

if (exitCode() != 0) {
LOGGER.atWarn()
.setMessage("Call failed with exitCode: {}\n\tstandardOutput: {}\n\tstandardError: {}")
.addArgument(this::exitCode)
.addArgument(standardOutput)
.addArgument(standardError)
.log();
LOGGER.warn(
"Call failed with exitCode: {}\n\tstandardOutput: {}\n\tstandardError: {}",
exitCode(),
standardOutput,
standardError);

throw new HelmExecutionException(exitCode(), standardError, standardOutput);
}
Expand Down
74 changes: 54 additions & 20 deletions ...client/src/main/java/com/hedera/fullstack/helm/client/execution/HelmExecutionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@

package com.hedera.fullstack.helm.client.execution;

import com.hedera.fullstack.base.api.collections.KeyValuePair;
import com.hedera.fullstack.helm.client.HelmConfigurationException;
import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
import java.util.*;
Expand All @@ -28,7 +30,8 @@
*/
public final class HelmExecutionBuilder {
private static final Logger LOGGER = LoggerFactory.getLogger(HelmExecutionBuilder.class);

public static final String NAME_MUST_NOT_BE_NULL = "name must not be null";
public static final String VALUE_MUST_NOT_BE_NULL = "value must not be null";
/**
* The path to the helm executable.
*/
Expand All @@ -42,7 +45,12 @@ public final class HelmExecutionBuilder {
/**
* The arguments to be passed to the helm command.
*/
private final Map<String, String> arguments;
private final HashMap<String, String> arguments;

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

@leninmehedy leninmehedy Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* The flags to be passed to the helm command.
Expand Down Expand Up @@ -73,10 +81,15 @@ public HelmExecutionBuilder(final Path helmExecutable) {
this.helmExecutable = Objects.requireNonNull(helmExecutable, "helmExecutable must not be null");
this.subcommands = new ArrayList<>();
this.arguments = new HashMap<>();
this.optionsWithMultipleValues = new ArrayList<>();
this.positionals = new ArrayList<>();
this.flags = new ArrayList<>();
this.environmentVariables = new HashMap<>();
this.workingDirectory = this.helmExecutable.getParent();

String workingDirectoryString = System.getenv("PWD");
this.workingDirectory = (workingDirectoryString == null || workingDirectoryString.isBlank())
? this.helmExecutable.getParent()
: new File(workingDirectoryString).toPath();
}

/**
Expand All @@ -100,12 +113,28 @@ public HelmExecutionBuilder subcommands(final String... commands) {
* @throws NullPointerException if either {@code name} or {@code value} is {@code null}.
*/
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");
Objects.requireNonNull(name, NAME_MUST_NOT_BE_NULL);
Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
this.arguments.put(name, value);
return this;
}

/**
* Adds an option with a provided list of values to the helm command. This is used for options that have can have
* multiple values for a single option. (e.g. --set and --values)
*
* @param name the name of the option.
* @param value the list of values for the given option.
* @return this builder.
* @throws NullPointerException if either {@code name} or {@code value} is {@code null}.
*/
public HelmExecutionBuilder optionsWithMultipleValues(final String name, final List<String> value) {
Objects.requireNonNull(name, NAME_MUST_NOT_BE_NULL);
Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
this.optionsWithMultipleValues.add(new KeyValuePair<>(name, value));
return this;
}

/**
* Adds a positional argument to the helm command.
*
Expand All @@ -114,7 +143,7 @@ public HelmExecutionBuilder argument(final String name, final String value) {
* @throws NullPointerException if {@code value} is {@code null}.
*/
public HelmExecutionBuilder positional(final String value) {
Objects.requireNonNull(value, "value must not be null");
Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
this.positionals.add(value);
return this;
}
Expand All @@ -128,20 +157,20 @@ public HelmExecutionBuilder positional(final String value) {
* @throws NullPointerException if either {@code name} or {@code value} is {@code null}.
*/
public HelmExecutionBuilder environmentVariable(final String name, final String value) {
Objects.requireNonNull(name, "name must not be null");
Objects.requireNonNull(value, "value must not be null");
Objects.requireNonNull(name, NAME_MUST_NOT_BE_NULL);
Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
this.environmentVariables.put(name, value);
return this;
}

/**
* Sets the working directory for the helm process.
* Sets the Path of the working directory for the helm process.
*
* @param workingDirectory the working directory.
* @param workingDirectoryPath the Path of the working directory.
* @return this builder.
*/
public HelmExecutionBuilder workingDirectory(final Path workingDirectory) {
this.workingDirectory = Objects.requireNonNull(workingDirectory, "workingDirectory must not be null");
public HelmExecutionBuilder workingDirectory(final Path workingDirectoryPath) {
this.workingDirectory = Objects.requireNonNull(workingDirectoryPath, "workingDirectoryPath must not be null");
return this;
}

Expand Down Expand Up @@ -191,18 +220,23 @@ private String[] buildCommand() {
command.addAll(subcommands);
command.addAll(flags);

for (final Map.Entry<String, String> entry : arguments.entrySet()) {
command.add(String.format("--%s", entry.getKey()));
command.add(entry.getValue());
}
arguments.forEach((key, value) -> {
command.add(String.format("--%s", key));
command.add(value);
});

optionsWithMultipleValues.forEach(entry -> entry.value().forEach(value -> {
command.add(String.format("--%s", entry.key()));
command.add(value);
}));

command.addAll(positionals);

String[] commandArray = command.toArray(new String[0]);
LOGGER.atDebug()
.setMessage("Helm command: {}")
.addArgument(String.join(" ", Arrays.copyOfRange(commandArray, 1, commandArray.length)))
.log();
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(
"Helm command: {}", String.join(" ", Arrays.copyOfRange(commandArray, 1, commandArray.length)));
}

return commandArray;
}
Expand Down
7 changes: 0 additions & 7 deletions fullstack-helm-client/src/main/java/com/hedera/fullstack/helm/client/model/Chart.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,12 @@

package com.hedera.fullstack.helm.client.model;

import java.util.Objects;

/**
* Represents a chart and is used to interact with the Helm install and uninstall commands.
* @param repoName the name of repository which contains the Helm chart.
* @param name the name of the Helm chart.
*/
public record Chart(String name, String repoName) {

public Chart {
Objects.requireNonNull(repoName, "repoName must not be null");
}

public Chart(String name) {
this(name, null);
}
Expand Down
11 changes: 9 additions & 2 deletions ...ent/src/main/java/com/hedera/fullstack/helm/client/model/install/InstallChartOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.hedera.fullstack.helm.client.execution.HelmExecutionBuilder;
import com.hedera.fullstack.helm.client.model.Options;
import java.util.List;

/**
* The options to be supplied to the helm install command.
Expand All @@ -32,6 +33,7 @@
* @param passCredentials - pass credentials to all domains.
* @param password - chart repository password where to locate the requested chart.
* @param repo - chart repository url where to locate the requested chart.
* @param set - set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)
* @param skipCrds - if set, no CRDs will be installed. By default, CRDs are installed if not already present.
* @param timeout - time to wait for any individual Kubernetes operation (like Jobs for hooks) (default 5m0s).
* @param username - chart repository username where to locate the requested chart.
Expand All @@ -54,10 +56,11 @@ public record InstallChartOptions(
boolean passCredentials,
String password,
String repo,
List<String> set,
boolean skipCrds,
String timeout,
String username,
String values,
List<String> values,
boolean verify,
String version,
boolean waitFor)
Expand Down Expand Up @@ -94,6 +97,10 @@ public void apply(final HelmExecutionBuilder builder) {
builder.argument("repo", repo());
}

if (set() != null) {
builder.optionsWithMultipleValues("set", set());
}

if (timeout() != null) {
builder.argument("timeout", timeout());
}
Expand All @@ -103,7 +110,7 @@ public void apply(final HelmExecutionBuilder builder) {
}

if (values() != null) {
builder.argument("values", values());
builder.optionsWithMultipleValues("values", values());
}

if (version() != null) {
Expand Down
19 changes: 17 additions & 2 deletions .../main/java/com/hedera/fullstack/helm/client/model/install/InstallChartOptionsBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.hedera.fullstack.helm.client.model.install;

import java.util.List;

/**
* The builder for the {@link InstallChartOptions}.
*/
Expand All @@ -29,10 +31,11 @@ public final class InstallChartOptionsBuilder {
private boolean passCredentials;
private String password;
private String repo;
private List<String> set;
private boolean skipCrds;
private String timeout;
private String username;
private String values;
private List<String> values;
private boolean verify;
private String version;
private boolean waitFor;
Expand Down Expand Up @@ -149,6 +152,17 @@ public InstallChartOptionsBuilder repo(String repo) {
return this;
}

/**
* set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)
*
* @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(List<String> valueOverride) {
this.set = valueOverride;
return this;
}

/**
* if set, no CRDs will be installed. By default, CRDs are installed if not already present.
*
Expand Down Expand Up @@ -188,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(String values) {
public InstallChartOptionsBuilder values(List<String> values) {
this.values = values;
return this;
}
Expand Down Expand Up @@ -247,6 +261,7 @@ public InstallChartOptions build() {
passCredentials,
password,
repo,
set,
skipCrds,
timeout,
username,
Expand Down
Loading