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 5 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
77 changes: 55 additions & 22 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 Set<KeyValuePair<String, String>> arguments;

/**
* The arguments of sets to be passed to the helm command.
*/
private final Set<KeyValuePair<String, Set<String>>> argumentsSet;

/**
* The flags to be passed to the helm command.
Expand Down Expand Up @@ -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<>();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

this.argumentsSet = new HashSet<>();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

? this.helmExecutable.getParent()
: new File(workingDirectoryString).toPath();
}

/**
Expand All @@ -100,9 +113,24 @@ 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");
this.arguments.put(name, value);
Objects.requireNonNull(name, NAME_MUST_NOT_BE_NULL);
Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
this.arguments.add(new KeyValuePair<>(name, value));
return this;
}

/**
* Adds an argument of sets to the helm command.
*
* @param name the name of the argument.
* @param value the set of value arguments.
* @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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Objects.requireNonNull(name, NAME_MUST_NOT_BE_NULL);
Objects.requireNonNull(value, VALUE_MUST_NOT_BE_NULL);
this.argumentsSet.add(new KeyValuePair<>(name, value));
return this;
}

Expand All @@ -114,7 +142,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 +156,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 +219,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(entry -> {
command.add(String.format("--%s", entry.key()));
command.add(entry.value());
});

argumentsSet.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.Set;

/**
* 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,
Set<String> set,
boolean skipCrds,
String timeout,
String username,
String values,
Set<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.argumentSet("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.argumentSet("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.Set;

/**
* 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 Set<String> set;
private boolean skipCrds;
private String timeout;
private String username;
private String values;
private Set<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(Set<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(Set<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