Skip to content

Introduce AbstractHelmMojo class #2714

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

Merged
manusa merged 3 commits into from
Mar 5, 2024
Merged

Introduce AbstractHelmMojo class #2714

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d76dfc
Introduce AbstractHelmMojo class
Jurrie Feb 21, 2024
c036268
Implement pull request comments
Jurrie Feb 22, 2024
faeb5d4
Add a quick note on the separate integration tests repository
Jurrie Mar 4, 2024
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
6 changes: 3 additions & 3 deletions ...kubernetes/src/main/java/org/eclipse/jkube/gradle/plugin/task/KubernetesHelmPushTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class KubernetesHelmPushTask extends AbstractJKubeTask {
@Inject
public KubernetesHelmPushTask(Class<? extends KubernetesExtension> extensionClass) {
super(extensionClass);
setDescription("Upload a helm chart to specified helm repository.");
setDescription("Upload a Helm chart to specified Helm repository.");
}

@Override
Expand All @@ -37,10 +37,10 @@ public void run() {
HelmConfig helm = initHelmConfig(kubernetesExtension.getDefaultHelmType(), kubernetesExtension.javaProject,
kubernetesExtension.getKubernetesTemplateOrDefault(),
kubernetesExtension.helm).build();
helm = initHelmPushConfig(helm, kubernetesExtension.javaProject);
initHelmPushConfig(helm, kubernetesExtension.javaProject);
jKubeServiceHub.getHelmService().uploadHelmChart(helm);
} catch (Exception exp) {
kitLogger.error("Error performing helm push", exp);
kitLogger.error("Error performing Helm push", exp);
throw new IllegalStateException(exp.getMessage(), exp);
}
}
Expand Down
4 changes: 3 additions & 1 deletion jkube-kit/helm/src/main/java/org/eclipse/jkube/kit/resource/helm/HelmConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.eclipse.jkube.kit.common.Maintainer;

import java.io.File;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -67,7 +68,8 @@ public class HelmConfig {
private String outputDir;
private String tarballOutputDir;
private String tarFileClassifier;
private List<GeneratedChartListener> generatedChartListeners;
@Builder.Default
private List<GeneratedChartListener> generatedChartListeners = new ArrayList<>();
private HelmRepository stableRepository;
private HelmRepository snapshotRepository;
private String security;
Expand Down
7 changes: 1 addition & 6 deletions jkube-kit/helm/src/main/java/org/eclipse/jkube/kit/resource/helm/HelmServiceUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,16 +119,11 @@ public static HelmConfig.HelmConfigBuilder initHelmConfig(
return helmConfig.toBuilder();
}

public static HelmConfig initHelmPushConfig(HelmConfig helmConfig, JavaProject project) {
if (helmConfig == null) {
helmConfig = new HelmConfig();
}

public static void initHelmPushConfig(HelmConfig helmConfig, JavaProject project) {
helmConfig.setStableRepository(initHelmRepository(helmConfig.getStableRepository(), project, STABLE_REPOSITORY));
helmConfig.setSnapshotRepository(initHelmRepository(helmConfig.getSnapshotRepository(), project, SNAPSHOT_REPOSITORY));

helmConfig.setSecurity(resolveFromPropertyOrDefault(PROPERTY_SECURITY, project, helmConfig::getSecurity, () -> DEFAULT_SECURITY));
return helmConfig;
}

static HelmRepository initHelmRepository(HelmRepository helmRepository, JavaProject project, String repositoryType) {
Expand Down
62 changes: 62 additions & 0 deletions ...ugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/AbstractHelmMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright (c) 2019 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at:
*
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.jkube.maven.plugin.mojo.build;

import static org.eclipse.jkube.kit.resource.helm.HelmServiceUtil.initHelmConfig;

import java.io.File;
import java.io.IOException;

import org.apache.maven.plugin.MojoFailureException;
import org.apache.maven.plugins.annotations.Parameter;
import org.eclipse.jkube.kit.resource.helm.HelmConfig;

public abstract class AbstractHelmMojo extends AbstractJKubeMojo {

/**
* One of:
* <ul>
* <li>A directory containing OpenShift Templates to use as Helm parameters.</li>
* <li>A file containing a Kubernetes List with OpenShift Template entries to be used as Helm parameters.</li>
* </ul>
*/
@Parameter(property = "jkube.kubernetesTemplate", defaultValue = "${basedir}/target/classes/META-INF/jkube/kubernetes")
File kubernetesTemplate;

@Parameter
HelmConfig helm;

@Override
public void init() throws MojoFailureException {
super.init();

try {
helm = initHelmConfig(getDefaultHelmType(), javaProject, getKubernetesTemplate(), helm).build();
} catch (IOException e) {
throw new MojoFailureException(e.getMessage(), e);
}
}

protected File getKubernetesTemplate() {
return kubernetesTemplate;
}

protected HelmConfig.HelmType getDefaultHelmType() {
return HelmConfig.HelmType.KUBERNETES;
}

protected HelmConfig getHelm() {
return helm;
}
}
22 changes: 11 additions & 11 deletions ...gin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/AbstractJKubeMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,22 +154,22 @@ public abstract class AbstractJKubeMojo extends AbstractMojo implements KitLogge

@Override
public void execute() throws MojoExecutionException, MojoFailureException {
try {
init();
if (shouldSkip()) {
log.info("`%s` goal is skipped.", mojoExecution.getMojoDescriptor().getFullGoalName());
return;
}
executeInternal();
} catch (DependencyResolutionRequiredException e) {
throw new MojoFailureException(e.getMessage());
init();
if (shouldSkip()) {
log.info("`%s` goal is skipped.", mojoExecution.getMojoDescriptor().getFullGoalName());
return;
}
executeInternal();
}

protected void init() throws DependencyResolutionRequiredException {
protected void init() throws MojoFailureException {
log = createLogger(null);
clusterAccess = new ClusterAccess(initClusterConfiguration());
javaProject = MavenUtil.convertMavenProjectToJKubeProject(project, session);
try {
javaProject = MavenUtil.convertMavenProjectToJKubeProject(project, session);
} catch (DependencyResolutionRequiredException e) {
throw new MojoFailureException(e.getMessage());
}
jkubeServiceHub = initJKubeServiceHubBuilder(javaProject).build();
resources = updateResourceConfigNamespace(namespace, resources);
}
Expand Down
15 changes: 2 additions & 13 deletions ...n-plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/HelmLintMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,11 @@
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.ResolutionScope;

import java.io.IOException;

import static org.eclipse.jkube.kit.resource.helm.HelmServiceUtil.initHelmConfig;

@Mojo(name = "helm-lint", defaultPhase = LifecyclePhase.INTEGRATION_TEST, requiresDependencyResolution = ResolutionScope.COMPILE)
public class HelmLintMojo extends HelmMojo {
public class HelmLintMojo extends AbstractHelmMojo {

@Override
public void executeInternal() throws MojoExecutionException {
try {
helm = initHelmConfig(getDefaultHelmType(), javaProject, getKubernetesTemplate(), helm)
.build();
jkubeServiceHub.getHelmService().lint(helm);
} catch (IOException e) {
throw new MojoExecutionException(e.getMessage(), e);
}

jkubeServiceHub.getHelmService().lint(getHelm());
}
}
65 changes: 24 additions & 41 deletions ...maven-plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/HelmMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,81 +15,64 @@

import java.io.File;
import java.io.IOException;
import java.util.Collections;

import org.eclipse.jkube.kit.resource.helm.HelmConfig;

import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugin.MojoFailureException;
import org.apache.maven.plugins.annotations.Component;
import org.apache.maven.plugins.annotations.LifecyclePhase;
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.Parameter;
import org.apache.maven.project.MavenProjectHelper;

import static org.eclipse.jkube.kit.resource.helm.HelmServiceUtil.initHelmConfig;
import org.eclipse.jkube.kit.resource.helm.GeneratedChartListener;
import org.eclipse.jkube.kit.resource.helm.HelmConfig;

/**
* Generates a Helm chart for the kubernetes resources
* Generates a Helm chart for the Kubernetes resources
*/
@Mojo(name = "helm", defaultPhase = LifecyclePhase.PRE_INTEGRATION_TEST)
public class HelmMojo extends AbstractJKubeMojo {

@Component
MavenProjectHelper projectHelper;
public class HelmMojo extends AbstractHelmMojo {

/**
* The generated kubernetes YAML file
* The generated Kubernetes YAML file
*/
@Parameter(property = "jkube.kubernetesManifest", defaultValue = "${basedir}/target/classes/META-INF/jkube/kubernetes.yml")
File kubernetesManifest;
private File kubernetesManifest;

/**
* One of:
* <ul>
* <li>A directory containing OpenShift Templates to use as Helm parameters.</li>
* <li>A file containing a Kubernetes List with OpenShift Template entries to be used as Helm parameters.</li>
* </ul>
*/
@Parameter(property = "jkube.kubernetesTemplate", defaultValue = "${basedir}/target/classes/META-INF/jkube/kubernetes")
File kubernetesTemplate;
@Component
MavenProjectHelper projectHelper;

@Parameter
HelmConfig helm;
@Override
public void init() throws MojoFailureException {
super.init();

final File manifest = getKubernetesManifest();
if (manifest == null || !manifest.isFile()) {
logManifestNotFoundWarning(manifest);
}

final GeneratedChartListener generatedChartListener = (helmConfig, type, chartFile) -> projectHelper.attachArtifact(project, helmConfig.getChartExtension(), type.getClassifier(), chartFile);
getHelm().getGeneratedChartListeners().add(generatedChartListener);
}

@Override
public void executeInternal() throws MojoExecutionException {
try {
final File manifest = getKubernetesManifest();
if (manifest == null || !manifest.isFile()) {
logManifestNotFoundWarning(manifest);
}
helm = initHelmConfig(getDefaultHelmType(), javaProject, getKubernetesTemplate(), helm)
.generatedChartListeners(Collections.singletonList((helmConfig, type, chartFile) -> projectHelper
.attachArtifact(project, helmConfig.getChartExtension(), type.getClassifier(), chartFile)))
.build();
jkubeServiceHub.getHelmService().generateHelmCharts(helm);
jkubeServiceHub.getHelmService().generateHelmCharts(getHelm());
} catch (IOException exception) {
throw new MojoExecutionException(exception.getMessage());
}
}

protected void logManifestNotFoundWarning(File manifest) {
getKitLogger().warn("No kubernetes manifest file has been generated yet by the k8s:resource goal at: " + manifest);
getKitLogger().warn("No Kubernetes manifest file has been generated yet by the k8s:resource goal at: " + manifest);
}

protected File getKubernetesManifest() {
return kubernetesManifest;
}

protected File getKubernetesTemplate() {
return kubernetesTemplate;
}

@Override
protected HelmConfig.HelmType getDefaultHelmType() {
return HelmConfig.HelmType.KUBERNETES;
}

HelmConfig getHelm() {
return helm;
}
}
22 changes: 12 additions & 10 deletions ...n-plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/HelmPushMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package org.eclipse.jkube.maven.plugin.mojo.build;

import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugin.MojoFailureException;
import org.apache.maven.plugins.annotations.LifecyclePhase;
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.ResolutionScope;
Expand All @@ -22,24 +23,25 @@
import static org.eclipse.jkube.kit.resource.helm.HelmServiceUtil.initHelmPushConfig;

@Mojo(name = "helm-push", defaultPhase = LifecyclePhase.INSTALL, requiresDependencyResolution = ResolutionScope.COMPILE)
public class HelmPushMojo extends HelmMojo {
public class HelmPushMojo extends AbstractHelmMojo {

@Override
public void init() throws MojoFailureException {
super.init();

initHelmPushConfig(helm, javaProject);
}

@Override
public void executeInternal() throws MojoExecutionException {
if (skip) {
return;
}
try {
super.executeInternal();
helm = initHelmPushConfig(helm, javaProject);
if (securityDispatcher instanceof DefaultSecDispatcher) {
((DefaultSecDispatcher) securityDispatcher).setConfigurationFile(helm.getSecurity());
((DefaultSecDispatcher) securityDispatcher).setConfigurationFile(getHelm().getSecurity());
}
jkubeServiceHub.getHelmService().uploadHelmChart(helm);
jkubeServiceHub.getHelmService().uploadHelmChart(getHelm());
} catch (Exception exp) {
getKitLogger().error("Error performing helm push", exp);
getKitLogger().error("Error performing Helm push", exp);
throw new MojoExecutionException(exp.getMessage(), exp);
}
}

}
24 changes: 6 additions & 18 deletions ...plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/OpenshiftHelmLintMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,22 @@
import org.apache.maven.plugins.annotations.Parameter;
import org.apache.maven.plugins.annotations.ResolutionScope;
import org.eclipse.jkube.kit.resource.helm.HelmConfig;
import org.eclipse.jkube.maven.plugin.mojo.OpenShift;

import java.io.File;

@Mojo(name = "helm-lint", defaultPhase = LifecyclePhase.INTEGRATION_TEST, requiresDependencyResolution = ResolutionScope.COMPILE)
public class OpenshiftHelmLintMojo extends HelmLintMojo {

/**
* The generated kubernetes YAML file
* One of:
* <ul>
* <li>A directory containing OpenShift Templates to use as Helm parameters.</li>
* <li>A file containing a Kubernetes List with OpenShift Template entries to be used as Helm parameters.</li>
* </ul>
*/
@Parameter(property = "jkube.kubernetesManifest", defaultValue = "${basedir}/target/classes/META-INF/jkube/openshift.yml")
private File openShiftManifest;

/**
* The generated kubernetes YAML file
*/
@Parameter(property = "jkube.kubernetesManifest", defaultValue = "${basedir}/target/classes/META-INF/jkube/openshift")
@Parameter(property = "jkube.openshiftTemplate", defaultValue = "${basedir}/target/classes/META-INF/jkube/openshift")
private File openShiftTemplate;

@Override
protected File getKubernetesManifest() {
return openShiftManifest;
}

@Override
protected File getKubernetesTemplate() {
Expand All @@ -51,9 +44,4 @@ protected File getKubernetesTemplate() {
protected HelmConfig.HelmType getDefaultHelmType() {
return HelmConfig.HelmType.OPENSHIFT;
}

@Override
protected String getLogPrefix() {
return OpenShift.DEFAULT_LOG_PREFIX;
}
Copy link
Member

@manusa manusa Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The OpenShift overrides are required to preserve the OpenShift-specific behavior (in this case the logged statements should be prefixed with oc: as opposed to k8s: same happens with the other apparently redundant overrides.

Copy link
Contributor Author

@Jurrie Jurrie Feb 22, 2024

Choose a reason for hiding this comment

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

Ah, I missed that this reads OpenShift :) I reverted this change in c036268.

Copy link
Contributor Author

@Jurrie Jurrie Feb 22, 2024

Choose a reason for hiding this comment

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

As for the other apparently redundant overrides: I feel that they are actually redundant. (#2714 (comment) for example.) But of course I might be wrong 😀

Copy link
Member

@manusa manusa Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

#2714 (comment)

Basically there are 4 things to override for the OpenShift Helm experience.

  • Static literal for the logger
  • Default enum value for the helm type
  • getKubernetesManifest: should respond to the user-provided property or configuration openShiftManifest instead
  • getKubernetesTemplate: should respond to the user-provided property or configuration openshiftTemplate instead (note that there is a bug in the current OpenshiftHelmPushMojo -Already taken care of pull/2714/files#r1498286179-) in the property, it reads "jkube.kubernetesManifest" when it should be `"jkube.openshiftTemplate""

As for the last two, I assume they were introduced to be able to override the default value of the parameter.

The only one redundant or unnecessary for the Helm Push mojo is the one for the kubernetesManifest, which is used for the warning log. (As a user, I want to get warned if I run the helm goal without having run the resource goal before, so that I'm aware of my misconfiguration). However, this warning should also apply for the Lint mojo.

The new AbstractHelmMojo has kubernetesTemplate, so this override should be kept.

Copy link
Contributor Author

@Jurrie Jurrie Mar 4, 2024

Choose a reason for hiding this comment

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

This holds for all OpenShift Helm classes that are changed, so for:

  • OpenshiftHelmLintMojo
  • OpenshiftHelmMojo
  • OpenshiftHelmPushMojo

Let's be methodical and list per method-to-be-overridden here:

getKubernetesManifest()

The getKubernetesManifest() method is defined in org.eclipse.jkube.maven.plugin.mojo.build.HelmMojo on line 70: https://github.com/eclipse/jkube/pull/2714/files#diff-1e94b9a102c6e2c667d4ee4da8631008a2237cb933ceff7bb0d365fe945b6fc0R70. Because it's available in HelmMojo, it can be overridden in OpenshiftHelmMojo.
Put differently; because it is not available in org.eclipse.jkube.maven.plugin.mojo.build.AbstractHelmMojo, it can not be overridden in HelmLintMojo or in OpenshiftHelmLintMojo, and it can not be overridden in HelmPushMojo or in OpenshiftHelmPushMojo.

The Kubernetes manifest property is only used in HelmMojo to generate a warning when k8s:resource or oc:resource is not ran. Do you think that the same warning should be shown when k8s:helm-lint, oc:helm-lint, k8s:helm-push or oc:helm-push is ran?

getKubernetesTemplate()

getDefaultHelmType()

getLogPrefix()

Copy link
Member

@manusa manusa Mar 5, 2024

Choose a reason for hiding this comment

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

This sounds about right.

The Kubernetes manifest property is only used in HelmMojo to generate a warning when k8s:resource or oc:resource is not ran. Do you think that the same warning should be shown when k8s:helm-lint, oc:helm-lint, k8s:helm-push or oc:helm-push is ran?

This is necessary for those goals that depend on the resources. So only for the initial HelmMojo. However, for the others, it'd be nice to have a similar warning reminding user to run the helm goal first (follow-up PR), the Gradle task behavior should be aligned too (follow-up PR 2).

}
4 changes: 2 additions & 2 deletions ...gin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/OpenshiftHelmMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
public class OpenshiftHelmMojo extends HelmMojo {

/**
* The generated kubernetes YAML file
* The generated Kubernetes YAML file
*/
@Parameter(property = "jkube.openshiftManifest", defaultValue = "${basedir}/target/classes/META-INF/jkube/openshift.yml")
private File openShiftManifest;
Expand Down Expand Up @@ -62,6 +62,6 @@ protected String getLogPrefix() {

@Override
protected void logManifestNotFoundWarning(File manifest) {
getKitLogger().warn("No openshift manifest file has been generated yet by the oc:resource goal at: " + manifest);
getKitLogger().warn("No OpenShift manifest file has been generated yet by the oc:resource goal at: " + manifest);
}
}
Loading