feat(manifests/helmfile): add helmfile templating engine #986

thameezb · 2023-05-23T15:16:21Z

Adds in support for helmfile as a "Render Engine" in the "Bake (Manifest)" stage.

This allows for existing helm configuration to still be used even when using helmfile (including additional value artifacts and explicit overrides) - fully debatable if this functionality should be allowed or not

Allows for usage with either helm2 or helm3 as the underlying templating engine - once again this is debatable if actually required

TODO:

add helmfile installation to docker files
add code to implement helmfile support
tests

feat(manifests/helmfile): add tests

dbyron-sf · 2023-06-15T19:37:02Z

Dockerfile.slim

@@ -4,6 +4,7 @@ LABEL maintainer="sig-platform@spinnaker.io"
 ENV KUSTOMIZE_VERSION=3.8.6
 ENV KUSTOMIZE4_VERSION=4.5.5
 ENV PACKER_VERSION=1.8.1
+ENV HELMFILE_VERSION=0.153.1


I noticed that 0.154.0 has now appeared. I'm OK to go ahead with 0.153.1 and update in a follow-up PR.

Since this PR, 0.155.0 has come out. Once this will be merged we are going to bump the version of helmfile.

dbyron-sf · 2023-06-15T19:39:05Z

.../java/com/netflix/spinnaker/rosco/manifests/config/RoscoHelmfileConfigurationProperties.java

+@ConfigurationProperties("helmfile")
+@Data
+public class RoscoHelmfileConfigurationProperties {
+ private String ExecutablePath = "helmfile";


Suggested change

private String ExecutablePath = "helmfile";

private String executablePath = "helmfile";

dbyron-sf · 2023-06-15T19:42:27Z

...ests/src/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileTemplateUtils.java

+ return "Failed to fetch helmfile " + description + ": " + e.getMessage();
+ }
+
+ public String removeTestsDirectoryTemplates(String inputString) {


Let's figure out a way to reduce the duplication / share the code in HelmTemplateUtils.

made a proposal for the this problem in here #986 (comment)

Moved this method under an abstract class.

dbyron-sf · 2023-06-15T19:43:04Z

...rc/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileBakeManifestRequest.java

@@ -0,0 +1,18 @@
+package com.netflix.spinnaker.rosco.manifests.helmfile;


License header, here and elsewhere.

dbyron-sf · 2023-06-15T19:43:28Z

.../src/test/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileTemplateUtilsTest.java

@@ -0,0 +1,674 @@
+/*
+ * Copyright 2019 Google, LLC


Suggested change

* Copyright 2019 Google, LLC

* Copyright 2023 Grab Holdings, Inc.

dbyron-sf · 2023-06-15T19:43:50Z

.../java/com/netflix/spinnaker/rosco/manifests/config/RoscoHelmfileConfigurationProperties.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright 2020 Grab Holdings, Inc.


Suggested change

* Copyright 2020 Grab Holdings, Inc.

* Copyright 2023 Grab Holdings, Inc.

dbyron-sf · 2023-06-15T19:48:21Z

.../src/test/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileTemplateUtilsTest.java

+import org.junit.runner.RunWith;
+import org.springframework.http.HttpStatus;
+
+@RunWith(JUnitPlatform.class)


No need for this line. Once it's gone, the imports can go too.

dbyron-sf · 2023-06-15T19:50:50Z

...rc/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileBakeManifestRequest.java

+ private String environment;
+ private String namespace;
+
+ List<Artifact> inputArtifacts;


Please add javadoc for each member, but especially this one.

dbyron-sf · 2023-06-15T19:53:45Z

...rc/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileBakeManifestService.java

+ return supportedTemplates.contains(type);
+ }
+
+ public Artifact bake(HelmfileBakeManifestRequest helmfileBakeManifestRequest) throws IOException {


This is so close to HelmBakeManifestService. How can we share the code?

I would put this code within the new package suggested here com.netflix.spinnaker.rosco.manifests.utils.helm. what do you think?

Looking at the code this is a function a bit hard to reduce in an OOP. I would need to create a separate class (abstract) to share only thing function. happy to hear here any proposals.

dbyron-sf · 2023-06-15T19:55:48Z

...ests/src/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileTemplateUtils.java

+ throw new SpinnakerHttpException(fetchFailureMessage("values file", e), e);
+ } catch (IOException | SpinnakerException e) {
+ throw new IllegalStateException(fetchFailureMessage("values file", e), e);
+ }


From here up, this basically dupilcates HelmTemplateUtils.buildBakeRecipe. How can we share the code?

You are right. The HelmTemplateUtils is spread of duplications. What about to create a common utils class for both helm and helmfile baker? Not sure under which package to place though. Perhaps I would have to create a new package com.netflix.spinnaker.rosco.manifests.utils.helm. what do you think?

@dbyron-sf I've tried to make the HelmTemplateUtils and HelmfileTemplateUtils dryer reusing where I could the code. The only thing missing to reduce is https://github.com/thameezb/rosco/blob/7e1f5160aaa7459b22009484ea56c8b6dcd8aea9/rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileTemplateUtils.java#L55-L97 but I cannot find a good way to reduce everything when there are in the middle POJOs. do you have any better ideas?

I've also reduce another bit the code in this commit 256756c

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

nemesisOsorio · 2023-07-04T18:07:11Z

.../java/com/netflix/spinnaker/rosco/manifests/config/RoscoHelmfileConfigurationProperties.java

+
+@ConfigurationProperties("helmfile")
+@Data
+public class RoscoHelmfileConfigurationProperties {


Prefix Rosco is unnecessary

All the other files have the Rosco prefix. we have just used the same conventions as the other files.

nemesisOsorio · 2023-07-04T18:11:47Z

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/HelmBakeTemplateUtils.java

+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.*;


we should avoid *

nemesisOsorio · 2023-07-04T18:27:44Z

...ests/src/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileTemplateUtils.java

+ BakeRecipe result = new BakeRecipe();
+ result.setName(request.getOutputName());
+
+ Path helmfileFilePath;
+ List<Path> valuePaths;
+
+ List<Artifact> inputArtifacts = request.getInputArtifacts();
+ if (inputArtifacts == null || inputArtifacts.isEmpty()) {
+ throw new IllegalArgumentException("At least one input artifact must be provided to bake");
+ }
+
+ log.info("helmfileFilePath: '{}'", request.getHelmfileFilePath());
+ Artifact helmfileTemplateArtifact = inputArtifacts.get(0);
+ String artifactType = Optional.ofNullable(helmfileTemplateArtifact.getType()).orElse("");
+ if ("git/repo".equals(artifactType)) {
+ env.downloadArtifactTarballAndExtract(super.getArtifactDownloader(), helmfileTemplateArtifact);
+
+ // If there's no helmfile path specified, assume it lives in the root of
+ // the git/repo artifact.
+ helmfileFilePath =
+ env.resolvePath(Optional.ofNullable(request.getHelmfileFilePath()).orElse(""));
+ } else {
+ try {
+ helmfileFilePath = downloadArtifactToTmpFile(env, helmfileTemplateArtifact);
+ } catch (SpinnakerHttpException e) {
+ throw new SpinnakerHttpException(fetchFailureMessage("template", e), e);
+ } catch (IOException | SpinnakerException e) {
+ throw new IllegalStateException(fetchFailureMessage("template", e), e);
+ }
+ }


seems like this code is duplicated

Yes it is duplicated but it is not easy to reuse. Please if you can recommend any ways to reuse this code I would appreciate it. Thanks

You should be able to extract duplication either to a utilities class or similar in -core type projects. Possibly rewriting some of the inputs/outputs to be more "generic" on downloading to a tmp file. Note I'm PRETTY sure kustomize uses some of this same logic, so a generic "extract artifact to folder" utility that passes the resulting FS path.

I'd also argue for a chroot type setup where each one runs in isolation or you can hit some interesting concurrency issues (which we kinda already get with tmp file support.. but... I've been seeing more edge cases lately). Don't think THIS ask is doable, but moving to a "core" lib should be.

Thanks @jasonmcintosh to provide some ideas. I'm going to check definitely whether the "core" lib way is a doable thing. I would like to avoid to introduce heavy refactors but certainly at this point it is too early to say that. I'll get back here once I have a more concrete idea.

can we move this method(buildBakeRecipe) to HelmBakeTemplateUtils class and add an abstract method for the command(buildCommand, getCommand)?

@nemesisOsorio the whole buildBakeRecipe depends by request POJO which differs from Helm and Helmfile so moving buildBakeRecipe into HelmBakeTemplateUtils won't solve the problem.

With the last changes, everything should result in more dry and favorite reuse.

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

mazzy89 · 2023-07-10T10:08:49Z

@jasonmcintosh @nemesisOsorio @dbyron-sf Everything has been addressed. now the code should look better.

mazzy89 · 2023-07-12T21:18:19Z

@thanks @nemesisOsorio for approving.
The build has run but there are some issues. going to address the errors

cc @dbyron-sf

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

mazzy89 · 2023-07-13T17:03:11Z

@nemesisOsorio it's all green now. do you mind to start the merge process adding the proper label?

jasonmcintosh

Overall, there are things I could see being improved, but as an initial feature I like this. Some cleanup and code reuse which is nice, overall, a solid new addition. I'd love to see the build command to be... redone but that can be a later thing.

thameezb · 2023-07-14T21:09:52Z

Big thanks to @mazzy89 and @jasonmcintosh for getting this to the finish line 🚀

feat(manifests/helmfile): install helmfile binary

e4f9508

thameezb marked this pull request as draft May 23, 2023 15:16

thameezb changed the title ~~draft: feat(manifests/helmfile): add helmfile templating engine~~ feat(manifests/helmfile): add helmfile templating engine May 23, 2023

thameezb force-pushed the feat-6837/addHelmfileTemplatingEngine branch from bdc44b4 to 1ac6324 Compare May 24, 2023 13:40

This was referenced May 26, 2023

feat(orca-bakery/manfests): add helmfile support spinnaker/orca#4460

Merged

feat(stages/bakeManifests): add helmfile support spinnaker/deck#9998

Merged

thameezb force-pushed the feat-6837/addHelmfileTemplatingEngine branch from 1ac6324 to 7eab89f Compare June 5, 2023 15:13

feat(manifests/helmfile): add code

95ff86b

feat(manifests/helmfile): add tests

thameezb force-pushed the feat-6837/addHelmfileTemplatingEngine branch from 7eab89f to 95ff86b Compare June 7, 2023 12:07

thameezb marked this pull request as ready for review June 12, 2023 12:44

dbyron-sf reviewed Jun 15, 2023

View reviewed changes

dbyron-sf mentioned this pull request Jun 15, 2023

[FEATURE] Support helmfile as render engine in Spinnaker spinnaker/spinnaker#6837

Closed

mazzy89 added 5 commits June 20, 2023 11:19

copyright

30ad3e3

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

test changes

997a574

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

add javadoc

a67d35d

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

make the code dry

7e1f516

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

reduce value paths

256756c

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

nemesisOsorio reviewed Jul 4, 2023

View reviewed changes

jasonmcintosh and others added 3 commits July 5, 2023 09:21

Merge branch 'master' into feat-6837/addHelmfileTemplatingEngine

3985c81

avoid using wildcards

c0ceabc

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

reduce redundant code

f2ac9e0

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

increase reuse

f877ba0

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

nemesisOsorio approved these changes Jul 12, 2023

View reviewed changes

Merge branch 'master' into feat-6837/addHelmfileTemplatingEngine

7c469ed

mazzy89 and others added 3 commits July 13, 2023 09:57

run spotless

3e86a4b

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

run another round of spotless

54249e2

Signed-off-by: Salvatore Mazzarino <salvatoremazz@double.cloud>

Merge branch 'master' into feat-6837/addHelmfileTemplatingEngine

6997196

jasonmcintosh approved these changes Jul 14, 2023

View reviewed changes

jasonmcintosh added the ready to merge Approved and ready for merge label Jul 14, 2023

mergify bot added the auto merged label Jul 14, 2023

mergify bot merged commit e5ea778 into spinnaker:master Jul 14, 2023
4 checks passed

spinnakerbot added the target-release/1.32 label Jul 14, 2023

thameezb deleted the feat-6837/addHelmfileTemplatingEngine branch July 14, 2023 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(manifests/helmfile): add helmfile templating engine #986

feat(manifests/helmfile): add helmfile templating engine #986

thameezb commented May 23, 2023 •

edited

Loading

dbyron-sf Jun 15, 2023

mazzy89 Jul 12, 2023 •

edited

Loading

dbyron-sf Jun 15, 2023

dbyron-sf Jun 15, 2023

mazzy89 Jun 20, 2023

mazzy89 Jun 22, 2023

dbyron-sf Jun 15, 2023

dbyron-sf Jun 15, 2023 •

edited

Loading

dbyron-sf Jun 15, 2023

dbyron-sf Jun 15, 2023

dbyron-sf Jun 15, 2023

mazzy89 Jun 20, 2023

dbyron-sf Jun 15, 2023

mazzy89 Jun 20, 2023

mazzy89 Jun 22, 2023

dbyron-sf Jun 15, 2023

mazzy89 Jun 20, 2023 •

edited

Loading

mazzy89 Jun 22, 2023

mazzy89 Jun 22, 2023

nemesisOsorio Jul 4, 2023

mazzy89 Jul 10, 2023

nemesisOsorio Jul 4, 2023

nemesisOsorio Jul 4, 2023

mazzy89 Jul 4, 2023

jasonmcintosh Jul 5, 2023

mazzy89 Jul 5, 2023

nemesisOsorio Jul 6, 2023

mazzy89 Jul 10, 2023

mazzy89 Jul 10, 2023 •

edited

Loading

mazzy89 commented Jul 10, 2023

mazzy89 commented Jul 12, 2023

mazzy89 commented Jul 13, 2023

jasonmcintosh left a comment

thameezb commented Jul 14, 2023

	private String ExecutablePath = "helmfile";
	private String executablePath = "helmfile";

		@@ -0,0 +1,18 @@
		package com.netflix.spinnaker.rosco.manifests.helmfile;

	* Copyright 2019 Google, LLC
	* Copyright 2023 Grab Holdings, Inc.

	* Copyright 2020 Grab Holdings, Inc.
	* Copyright 2023 Grab Holdings, Inc.

feat(manifests/helmfile): add helmfile templating engine #986

feat(manifests/helmfile): add helmfile templating engine #986

Conversation

thameezb commented May 23, 2023 • edited Loading

Choose a reason for hiding this comment

mazzy89 Jul 12, 2023 • edited Loading

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

dbyron-sf Jun 15, 2023 • edited Loading

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

mazzy89 Jun 20, 2023 • edited Loading

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

mazzy89 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

mazzy89 commented Jul 10, 2023

mazzy89 commented Jul 12, 2023

mazzy89 commented Jul 13, 2023

jasonmcintosh left a comment

Choose a reason for hiding this comment

thameezb commented Jul 14, 2023

thameezb commented May 23, 2023 •

edited

Loading

mazzy89 Jul 12, 2023 •

edited

Loading

dbyron-sf Jun 15, 2023 •

edited

Loading

mazzy89 Jun 20, 2023 •

edited

Loading

mazzy89 Jul 10, 2023 •

edited

Loading