fix(helm): teach rosco to write helm overrides to a file #1135
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbyron-sf
force-pushed
the
write_helm_overrides_to_file
branch
from
2e5587d to
88193c8
Compare
xibz
reviewed
Jan 26, 2025
| @@ -126,12 +126,17 @@ protected List<String> buildOverrideList(Map<String, Object> overrides) { | |||
| .collect(Collectors.toList()); | |||
| } | |||
|
|
|||
| /** Accessor for whether to expand artifact reference URIs */ | |||
| protected boolean isExpandArtifactReferenceURIs() { | |||
| return (artifactStore != null && helmConfig.isExpandOverrides()); | |||
There was a problem hiding this comment.
Can you explain the use case for this a little bit? Im unsure why you want to not expand something in rosco? Or maybe Im misunderstanding how this works.
There was a problem hiding this comment.
Before this change, artifact references would sometimes remain in the values file, e.g.:
Created overrides file at /tmp/rosco-9390928878072864271/overrides_d07566cf-e592-4b43-ad94-d947782f4a88.yml with the following contents:
---
my-override: "ref://dbyrontest/7678d0be07b31c883c436a70522c58ec0f724f1bf8bbeb293165e330f495e321"
There was a problem hiding this comment.
The override in question was configured as:
"my-override": "${#stage(\"Bake (Manifest)\").outputs.resolvedExpectedArtifacts[0].boundArtifact.reference}"
as in, there's a bake stage with an override whose value is the result of a prior bake stage.
xibz
suggested changes
Jan 26, 2025
| private static Stream<Arguments> helmParseArgs() { | ||
|
|
||
| return Stream.of( | ||
| Arguments.of("name1=value1", Map.of("name1", "value1"), true), |
There was a problem hiding this comment.
Turns out things are more complicated than we originally thought...
There was a problem hiding this comment.
The commit message for d8d08fb has the gory details.
xibz
approved these changes
Jan 26, 2025
The goal is to address an error in HelmTemplateUtils that occurs when the command line becomes too long due to a large number of overrides. The solution is to introduce a configurable threshold. If the overrides exceed this threshold, write them to a file and use the --values option when invoking Helm. The default behaviour (threshold of 0) retains the current approach of not using a file, regardless of the size of the overrides.
…elm template command line ensure override precedence in helm template by appending --values with overrides at the command end, as --set/--set-string override --values in Helm 2.16.1 and 3.4.1, and later --values take highest precedence
…erence URIs if configured to do so
When creating overrides YAML files with nested key structures, they are being represented as flattened rather than structured keys. This leads to discrepancies in the output of the helm template command when comparing the use of --set/--set-string versus --values with an overrides YAML. While --set/--set-string correctly renders keys with their nested structure, using --values with a YAML file resulted in the omission of these nested keys. This fixes that, so nested keys work. For example, if an override was specified as ``` overrides: image.tag: "foo" ``` before this PR, the value of image.tag wouldn't be "foo", it would be the default value in the helm chart. With this PR, the value of image.tag is "foo". A new HelmSetArgumentParser class inspired by Helm's Go code for parsing --set arguments handles complex nested structures, including lists and maps, and supports type inference for values.
…absolute value is >= 1,000,000 Helm 3 treats large numbers (greater than or equal to 1,000,000) as int64 when passed via --set, and as float64 when passed via --values. This behavior causes a difference in the template output, with large numeric values being output in non-scientific notation when using --set, and in scientific notation when using --values. To maintain consistent Helm 3 template behavior in Spinnaker, it is necessary to skip the overrides YAML file feature if any of the overrides' value is greater than or equal to 1,000,000.
…wnloaderTest to rosco-integration The tests V2BakeryControllerWithArtifactDownloaderTest depend on an executable named helm3 being in the path, and an executable named helm being helm v2. This isn't typically the case (anymore) on the local file system of developer machines. Moving the tests to rosco-integration means using the helm executables from the just-built docker image, where these dependencies are satisfied. As well, as we upgrade the versions of helm in rosco, we'll verify that override handling logic continues to function.
before this, the values file would contain a string, even if rawOverrides is true. In other words, `--set my-override=1.234` would become ``` my-override: "1.234" ``` After this change, it's: ``` my-override: 1.234 ```
…lues quoted
The reason to leave floats quoted is that it more closely matches helm's behavior.
Consider this helm chart:
```
apiVersion: v2
name: float-example
description: A Helm chart example that includes a float value
version: 0.1.0
appVersion: "1.0"
```
that has a values file internal to the chart (aka defaults) like:
```
threshold: 3.14
```
and
```
apiVersion: v1
kind: ConfigMap
metadata:
name: float-example-config
data:
# Using the float directly as a numeric value
numericThreshold: {{ .Values.threshold }}
# Converting the float to a string
stringThreshold: "{{ .Values.threshold }}"
# Alternatively, using Helm's `quote` function
quotedThreshold: {{ quote .Values.threshold }}
thresholdTypeSprig: "{{ typeOf .Values.threshold }}"
```
With both helm 3.13.1 and 3.16.2, look at the output of various commands:
```
$ helm template float-example ./float-example
---
apiVersion: v1
kind: ConfigMap
metadata:
name: float-example-config
data:
# Using the float directly as a numeric value
numericThreshold: 3.14
# Converting the float to a string
stringThreshold: "3.14"
# Alternatively, using Helm's `quote` function
quotedThreshold: "3.14"
thresholdTypeSprig: "float64"
```
So far so good: there's a float value in the chart, and it appears in the output as a float.
Now let's try overriding with an int value using --set:
```
$ helm template float-example ./float-example --set threshold=6
---
apiVersion: v1
kind: ConfigMap
metadata:
name: float-example-config
data:
# Using the float directly as a numeric value
numericThreshold: 6
# Converting the float to a string
stringThreshold: "6"
# Alternatively, using Helm's `quote` function
quotedThreshold: "6"
thresholdTypeSprig: "int64"
```
Still OK so far. Now the clincher, let's try a decimal value using --set:
```
$ helm template float-example ./float-example --set threshold=6.28
---
apiVersion: v1
kind: ConfigMap
metadata:
name: float-example-config
data:
# Using the float directly as a numeric value
numericThreshold: 6.28
# Converting the float to a string
stringThreshold: "6.28"
# Alternatively, using Helm's `quote` function
quotedThreshold: "6.28"
thresholdTypeSprig: "string"
```
The type has changed to a string!
to complete the story, using --values:
```
$ cat external-values.yaml
threshold: 6.28
```
$ helm template float-example ./float-example --values external-values.yaml
---
apiVersion: v1
kind: ConfigMap
metadata:
name: float-example-config
data:
# Using the float directly as a numeric value
numericThreshold: 6.28
# Converting the float to a string
stringThreshold: "6.28"
# Alternatively, using Helm's `quote` function
quotedThreshold: "6.28"
thresholdTypeSprig: "float64"
```
To summarize:
- default (in-chart) value of float yields a float
- using --set threshold=6.28 yields a string
- using --values external-values.yaml with
```
threshold: 6.28
```
yields a float
To tie this back to Spinnaker, an override in a helm bake manifest stage like:
```
threshold: 6.28
```
ends up as a `--set` option on the helm template command line before this PR, and so it's a string in the helm chart. To preserve that behavior when the length of overrides is over the threshold, this PR has:
```
threshold: "6.28"
```
in the generated values file.
Removing the quotes in the generated values file seems cleaner, but is a change in the current behavior. An alternate would be to add special logic to leave float overrides using `--set`. That's left for a future PR.
dbyron-sf
force-pushed
the
write_helm_overrides_to_file
branch
from
ac79f24 to
d8d08fb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
to address an error in HelmTemplateUtils that occurs when the command line becomes too long due to a large number of overrides. The solution is to introduce a configurable threshold (
helm.overridesFileThreshold). If the total length of override values exceeds this threshold, write them to a file and use the--valuesoption when invoking Helm. The default behaviour (threshold of 0) retains the current approach of not using a file, regardless of the length of the overrides.Note that there are a number of commits here, as we iterated on the fix internally. It turns out it's super complicated to get info in a values file with the same behavior in both helm v2 and helm v3 that we get with --set/--set-string. At this point it probably makes the most sense to look at all commits together, but I left them separate at least to start if folks are curious about the journey.