feat: support boolean and integer expansion#934
Conversation
| sb.WriteString(s) | ||
| // FIXME: this skips the rest of the string, is this intended? | ||
| return sb.String(), nil |
There was a problem hiding this comment.
It seems like the code should just skip to the end of the current match.
Something like this:
| sb.WriteString(s) | |
| // FIXME: this skips the rest of the string, is this intended? | |
| return sb.String(), nil | |
| sb.WriteString(s[:ixs[1]]) // Write as is from the beginning of the string to the end of the current match | |
| s = s[ixs[1]:] | |
| continue |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for boolean and integer value expansion from secret sources. Previously, only string values could be fetched from secret providers, and non-string values were skipped. The change enables type preservation so that boolean and integer values maintain their types in the output YAML/JSON instead of being converted to strings.
Key Changes:
- Modified the Lookup function signature to return
interface{}instead ofstringto support multiple value types - Added
isTerminalValuehelper to identify acceptable scalar types (bool, int, string) - Introduced
InValuefunction that preserves types for full-match expansions while converting partial matches to strings - Added test coverage for boolean and integer type preservation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| vals.go | Changed Lookup signature from string to interface{}, added isTerminalValue helper, updated type handling in switch statement to include bool/int |
| pkg/expansion/expand_match.go | Added InValue function for type-preserving expansion, extracted shouldExpand helper, updated InMap to use InValue |
| pkg/expansion/expand_match_test.go | Updated test signatures to match new Lookup interface{} return type |
| vals_test.go | Added TestEvalNodesTypes to verify bool/int type preservation; minor formatting improvements (octal literals, var declarations) |
| sb.WriteString(s) | ||
| // FIXME: this skips the rest of the string, is this intended? | ||
| return sb.String(), nil |
There was a problem hiding this comment.
This FIXME comment highlights a potential bug in the existing logic. When a reference kind is not in the expansion list, the function returns immediately, skipping any subsequent references in the same string. This means if you have multiple references in a string and the first one should be skipped, the remaining ones won't be processed either. Consider whether this behavior is intended or if the function should continue processing the rest of the string after skipping non-matching references.
| sb.WriteString(s) | |
| // FIXME: this skips the rest of the string, is this intended? | |
| return sb.String(), nil | |
| // Keep this reference as-is and continue processing the rest of the string. | |
| sb.WriteString(s[:ixs[1]]) | |
| s = s[ixs[1]:] | |
| continue |
| // InValue expands matches in the given string value. | ||
| // If the entire string matches the regex, it expands and preserves the type. | ||
| // If only part of the string matches, it expands as a string. | ||
| func (e *ExpandRegexMatch) InValue(s string) (interface{}, error) { | ||
| ixs := e.Target.FindStringSubmatchIndex(s) | ||
| switch { | ||
| // No match, return as is | ||
| case ixs == nil: | ||
| return s, nil | ||
| // Full match, expand preserving type | ||
| case ixs[0] == 0 && ixs[1] == len(s): | ||
| kind := s[ixs[2]:ixs[3]] | ||
| ref := s[ixs[6]:ixs[7]] | ||
| if !e.shouldExpand(kind) { | ||
| return s, nil | ||
| } | ||
| val, err := e.Lookup(ref) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("expand %s: %v", ref, err) | ||
| } | ||
| return val, nil | ||
| // Partial match, expand as string | ||
| default: | ||
| return e.InString(s) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new InValue function that enables type preservation for boolean and integer values lacks test coverage. Consider adding tests that verify type preservation for full matches and string conversion for partial matches, including edge cases like booleans, integers, and mixed scenarios.
vals.go
Outdated
| newobj := map[string]interface{}{} | ||
| switch t := obj[k].(type) { | ||
| case string: | ||
| case bool, int, string: |
There was a problem hiding this comment.
The type check only handles int, but JSON unmarshaling (used by providers like Scaleway, and potentially returned by other providers) produces float64 for all numeric values. This means integer values from JSON-based secret sources will not be recognized as terminal values and will cause errors. Consider adding support for float64, int64, int32, and other numeric types that might be returned by different providers and unmarshaling libraries.
| case bool, int, string: | |
| case bool, int, int32, int64, float32, float64, string: |
There was a problem hiding this comment.
This suggestion seems reasonable. Should we support more scalar types?
|
|
||
| func isTerminalValue(v any) bool { | ||
| switch v.(type) { | ||
| case bool, int, string: |
There was a problem hiding this comment.
The type check only handles int, but JSON unmarshaling (used by providers like Scaleway, and potentially returned by other providers) produces float64 for all numeric values. This means integer values from JSON-based secret sources will not be recognized as terminal values. Consider adding support for float64, int64, int32, and other numeric types that might be returned by different providers and unmarshaling libraries.
| case bool, int, string: | |
| case bool, | |
| int, int8, int16, int32, int64, | |
| uint, uint8, uint16, uint32, uint64, | |
| float32, float64, | |
| string: |
| func TestEvalNodesTypes(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| createTmpFile := func(t *testing.T, dir, name, content string) string { | ||
| tmpFilePath := filepath.Join(dir, name) | ||
| err := os.WriteFile(tmpFilePath, []byte(content), 0o600) | ||
| require.NoError(t, err) | ||
| return tmpFilePath | ||
| } | ||
|
|
||
| secretYaml := ` | ||
| bool: true | ||
| int: 42 | ||
| string: "It's a string" | ||
| ` | ||
| secretsFile := createTmpFile(t, tmpDir, "secrets.yaml", secretYaml) | ||
|
|
||
| replacer := strings.NewReplacer("{file-ref}", "ref+file://"+secretsFile) | ||
| inputYaml := replacer.Replace(` | ||
| bool_value: {file-ref}#/bool | ||
| int_value: {file-ref}#/int | ||
| string_value: {file-ref}#/string | ||
| `) | ||
| inputFile := createTmpFile(t, tmpDir, "input.yaml", inputYaml) | ||
|
|
||
| expected := `bool_value: true | ||
| int_value: 42 | ||
| string_value: It's a string | ||
| ` | ||
|
|
||
| input, err := Inputs(inputFile) | ||
| require.NoError(t, err) | ||
|
|
||
| nodes, err := EvalNodes(input, Options{}) | ||
| require.NoError(t, err) | ||
| buf := new(strings.Builder) | ||
|
|
||
| err = Output(buf, "", nodes) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, expected, buf.String()) | ||
| } |
There was a problem hiding this comment.
The test only covers YAML-based providers (file provider with .yaml file) which correctly produce int types. However, it doesn't test JSON-based providers (like Scaleway) that produce float64 for numeric values. Consider adding test coverage for JSON-based secret sources to ensure numeric type handling works correctly across all provider types.
|
@Zebradil please fix conficts |
This PR implements support for boolean and integer values in secret sources. Previously, only string values were read from secret sources, and non-string values were skipped, so this should not be a breaking change. Fixes helmfile#190 Related to helmfile#492 Signed-off-by: German Lashevich <german.lashevich@gmail.com>
Signed-off-by: German Lashevich <german.lashevich@gmail.com>
Signed-off-by: German Lashevich <german.lashevich@gmail.com>
Signed-off-by: German Lashevich <german.lashevich@gmail.com>
|
@yxxhero done |
This PR implements support for boolean and integer values in secret sources.
Previously, only string values were read from secret sources, and non-string
values were skipped, so this should not be a breaking change.
Fixes #190
Related to #492
I'm aware of the idea of having
as=intandas=boolURL parameters brought by @mumoshu in #492.It is not implemented in this PR, but it can be further discussed.
In my opinion, these parameters aren't strictly necessary for the initial goal of the PR to support non-string values in secret sources.
However, such parameters can be useful in case a user needs to convert types. This can also be implemented in a separate PR.
Furthermore, contrary to the approach taken in #492, this PR aims to preserve types instead of just supporting lookups of non-string secret values. This is because I, as a user, do not always control how expanded secret values are used, but I usually control how secret values are stored.
For example, I use
valsto inject secret values into a Kubernetes Custom Resource manifest (YAML). Since the officialvalssupports only strings, I can't inject integer or boolean values. The injection happens on the server-side and the results are not stored but applied immediately, so I don't have access to the manifests with injected secrets. The Custom Resource Definition requires the values to be integers instead of strings. This makes the manifests invalid, so they can't be used.P.S.: My formatter changed some unrelated code, I'll roll it back if needed.