feat: Preserve yaml formatting when writing back to helm values files#1489
feat: Preserve yaml formatting when writing back to helm values files#1489grahamalama wants to merge 7 commits intoargoproj-labs:masterfrom
Conversation
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
Add ValuesFile type that wraps goccy/go-yaml's AST to preserve comments, blank lines, and indentation when updating helm values files. Also add helmParameterYAML wrapper type with explicit yaml tags for backward compatibility with existing files that use lowercase field names. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
Replace manual yaml.Node manipulation with ValuesFile for format-preserving YAML operations. Switch from sigs.k8s.io/yaml/goyaml.v3 to goccy/go-yaml. Remove old setHelmValue/getHelmValue functions and their tests, now covered by ValuesFile tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
grahamalama
requested review from
chengfang,
dkarpele,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
WalkthroughAdds a format-preserving ValuesFile abstraction for YAML Helm values, replaces direct yaml.Node manipulation with ValuesFile APIs (Parse/Set/Get/Bytes), introduces YAML-specific Helm parameter types, switches serialization to github.com/goccy/go-yaml, and updates tests for formatting preservation and parameter sorting. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Updater as Update Logic
participant VF as ValuesFile
participant Parser as YAML Parser
participant AST as YAML AST
participant Ser as Serializer
User->>Updater: Request Helm values update
Updater->>VF: ParseValuesFile(data)
VF->>Parser: Parse with comment/format preservation
Parser->>AST: Build AST (with comments/anchors)
AST-->>VF: Parsed AST
Updater->>VF: SetValue(key, value)
VF->>AST: Navigate and modify nodes
AST-->>VF: Updated AST
Updater->>VF: Bytes()
VF->>Ser: Serialize preserving comments/blanks/indent
Ser-->>VF: YAML bytes
VF-->>Updater: Formatted YAML output
Updater-->>User: Return updated formatted file
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1489 +/- ##
==========================================
+ Coverage 71.48% 72.27% +0.79%
==========================================
Files 50 52 +2
Lines 4667 4989 +322
==========================================
+ Hits 3336 3606 +270
- Misses 1133 1169 +36
- Partials 198 214 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
grahamalama
force-pushed
the
preserve-yaml-formatting
branch
from
c542e44 to
13d768d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/argocd/values_file.go`:
- Around line 195-208: The code that handles missing path segments (in the block
using valueNode == nil inside the path traversal) currently always creates a
mapping or scalar via addKeyMapping/addKeyValue, which fails when the next key
is an array index like "images[0]"; update the logic in the traversal that
references keys, keyPart, i, current, addKeyMapping and addKeyValue to detect
array notation (e.g., keyPart contains bracket/index) and either (a) create an
explicit sequence node and populate the indexed entry (creating intermediate
mappings as needed) or (b) fail fast with a clear error when a missing array
index is encountered—choose one approach and implement it consistently so
missing "images[0]" creates a sequence (or returns an error) instead of a
mapping/scalar.
- Around line 47-64: The SetValue function currently returns an error when
doc.Body is nil (comment-only YAML); change it to initialize doc.Body to an
empty mapping node instead of erroring so comments are preserved and values can
be set. In the SetValue function, when detecting root == nil, create and assign
an empty mapping node to doc.Body (so subsequent rootMapping, root := doc.Body
and type-assert to *ast.MappingNode succeed) and proceed as normal; ensure you
reference the existing symbols y.file.Docs, doc.Body, and rootMapping in this
adjustment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/argocd/values_file.go`:
- Around line 49-73: Update GetValue to use the same lookup order as SetValue:
check for a literal key first using findKeyInMapping (or the same helper used in
SetValue) and return that value if found, otherwise fall back to navigating the
dotted path (the nested lookup used by setNestedValue/getNestedValue). Ensure
GetValue mirrors SetValue's control flow and helper calls so literal "image.tag"
wins over nested image->tag, maintaining round-trip consistency between SetValue
and GetValue.
- Around line 118-137: setNodeValue currently only accepts *ast.StringNode
causing SetValue to fail for integer/float/bool scalars; update setNodeValue
(and preserve behavior of unwrapNode) to accept and set *ast.IntegerNode,
*ast.FloatNode, and *ast.BoolNode in addition to *ast.StringNode by converting
the incoming value string to the appropriate Token/Value representation for each
node type and preserving comments via each node's SetComment, and when mutating
make sure to update both the node.Value field and the underlying
Token.Value/Token.Origin (or any token fields used by goccy/go-yaml
serialization) so emitted YAML reflects the new value; mirror getScalarValue's
type handling and reuse its logic for locating node types and comment
preservation.
🧹 Nitpick comments (1)
pkg/argocd/values_file.go (1)
385-386: Hardcoded 2-space child indent may clash with files using different indentation.
addKeyMappingalways adds 2 to the parent indent. If the existing file uses 4-space indentation, newly created intermediate keys will be indented inconsistently. Consider inferring the indent step from existing sibling or child nodes when available.
- Align GetValue and SetValue lookup order (literal key first, then nested) - Fail fast when array notation references missing keys - Support integer, float, and bool scalar value types - Handle comment-only YAML files gracefully - Infer indentation from existing file structure instead of hardcoding - Simplify code: use type switches, remove redundant conditions - Fix column positioning for correct YAML serialization Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
grahamalama
force-pushed
the
preserve-yaml-formatting
branch
from
00fbb3f to
170ea02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/argocd/values_file.go`:
- Around line 182-198: getScalarValue currently doesn't handle YAML nulls and
will return a "not a scalar" error for *ast.NullNode; add a NullNode branch in
getScalarValue (after the *ast.AliasNode case) to return the node's token value
(use n.GetToken().Value) so nulls like `null`, `~`, or empty `key:` are treated
as scalars; also confirm updateNodeToken works with *ast.NullNode (it should
since NullNode implements GetToken/GetComment) and adjust only if you encounter
token interface mismatches.
🧹 Nitpick comments (3)
pkg/argocd/values_file.go (2)
403-417: Newly created values are always unquoted strings — acceptable but worth noting.
addKeyValuealways creates anast.Stringnode for the value. If the original file uses quoted strings or if the value looks like a YAML special value (e.g.,"true","null","123"), the new key will be emitted unquoted, potentially changing the YAML type semantics. For the primary use case (image tags likev2.0.0), this is fine.
342-354: Potential nil-dereference chain ingetMappingIndent.Line 346:
mapping.Values[0].Key.GetToken().Position.IndentNum— ifKeyis nil (unlikely from a parser but possible for programmatically built ASTs), orGetToken()returns nil, this panics. The parser-generated AST should always have these set, and yourmakeToken/addKeyValuehelpers always create well-formed tokens, so the practical risk is low.pkg/argocd/values_file_test.go (1)
10-284: Good test coverage forSetValue— consider a few additional edge cases.The 16 subtests cover a solid range of scenarios including formatting preservation, type handling, indent inference, and error paths. A few notable gaps that could improve confidence:
- Null values: No test for
key: nullorkey: ~(relates to theNullNodehandling gap flagged in the implementation).- Array out-of-bounds: No test verifying the error when
arrayIdx >= len(seqNode.Values).CreateEmptyValuesFile+SetValue: The constructor is untested.Bytes(): OnlyString()is asserted;Bytes()is trivial but untested.These are not blockers but would help close the coverage gap the Codecov report flags (~79% patch coverage).
| // getScalarValue extracts the string value from a node. | ||
| func getScalarValue(node ast.Node) (string, error) { | ||
| switch n := node.(type) { | ||
| case *ast.AliasNode: | ||
| return getScalarValue(n.Value) | ||
| case *ast.StringNode: | ||
| return n.Value, nil | ||
| case *ast.IntegerNode: | ||
| return n.GetToken().Value, nil | ||
| case *ast.FloatNode: | ||
| return n.GetToken().Value, nil | ||
| case *ast.BoolNode: | ||
| return n.GetToken().Value, nil | ||
| default: | ||
| return "", fmt.Errorf("node is not a scalar value (type: %T)", node) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file and see the exact code at lines 182-198
if [ -f "pkg/argocd/values_file.go" ]; then
echo "=== File exists, showing lines 182-198 ==="
sed -n '182,198p' pkg/argocd/values_file.go
echo ""
echo "=== Full getScalarValue function ==="
rg -A 20 "func getScalarValue" pkg/argocd/values_file.go
else
echo "File not found"
fiRepository: argoproj-labs/argocd-image-updater
Length of output: 1234
🏁 Script executed:
#!/bin/bash
# Check for updateNodeToken function
echo "=== Looking for updateNodeToken function ==="
rg -n "func updateNodeToken" pkg/argocd/values_file.go
rg -A 15 "func updateNodeToken" pkg/argocd/values_file.goRepository: argoproj-labs/argocd-image-updater
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Search for any NullNode references in the codebase
echo "=== Searching for NullNode references ==="
rg -n "NullNode" pkg/argocd/Repository: argoproj-labs/argocd-image-updater
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check what version of goccy/go-yaml is being used and inspect its types
if [ -f "go.mod" ] || [ -f "go.sum" ]; then
echo "=== Checking go.mod/go.sum for goccy/go-yaml version ==="
grep -i "goccy/go-yaml" go.mod go.sum 2>/dev/null | head -5
fiRepository: argoproj-labs/argocd-image-updater
Length of output: 358
🏁 Script executed:
#!/bin/bash
# Search for any test files that might test null values
echo "=== Searching for tests related to null values ==="
rg -n "null|~" --type go pkg/argocd/ -i | grep -i test | head -20Repository: argoproj-labs/argocd-image-updater
Length of output: 618
🏁 Script executed:
#!/bin/bash
# Get the full updateNodeToken function
echo "=== Full updateNodeToken function ==="
rg -A 50 "func updateNodeToken" pkg/argocd/values_file.goRepository: argoproj-labs/argocd-image-updater
Length of output: 1566
🏁 Script executed:
#!/bin/bash
# Check the imports in the file to understand the ast package
echo "=== Imports in values_file.go ==="
head -30 pkg/argocd/values_file.go | grep -A 20 "^import"Repository: argoproj-labs/argocd-image-updater
Length of output: 630
🏁 Script executed:
#!/bin/bash
# Try to verify if NullNode exists by checking go.sum or looking at module
# Let's search for any documentation or indirect evidence
echo "=== Checking if goccy/go-yaml has NullNode ==="
rg "NullNode" . --type go 2>/dev/null || echo "No NullNode found"
</details>
# Also look at what types ARE handled
echo ""
echo "=== All ast types referenced in values_file.go ==="
rg "\*ast\." pkg/argocd/values_file.go | grep -o "\*ast\.[A-Za-z]*" | sort -uRepository: argoproj-labs/argocd-image-updater
Length of output: 263
🏁 Script executed:
#!/bin/bash
# Check if there's actual test data with null YAML values
echo "=== Looking for YAML test fixtures with null ==="
find . -name "*.yaml" -o -name "*.yml" | head -10 | xargs grep -l "null\|~" 2>/dev/null || echo "No YAML null values found in test fixtures"
# Also search for test data in test files
echo ""
echo "=== Searching test files for null value patterns ==="
rg ":\s*null|:\s*~" --type go pkg/argocd/ -A 2 -B 2 | head -30Repository: argoproj-labs/argocd-image-updater
Length of output: 1514
🏁 Script executed:
#!/bin/bash
# Check what node types are handled elsewhere in the codebase
echo "=== All node type checks in values_file.go ==="
rg "case \*ast\." pkg/argocd/values_file.go
echo ""
echo "=== Check for any nil/null value handling ==="
rg "Null|nil" pkg/argocd/values_file.go | head -20Repository: argoproj-labs/argocd-image-updater
Length of output: 1347
🌐 Web query:
goccy/go-yaml v1.19.2 ast.NullNode type
💡 Result:
In github.com/goccy/go-yaml/ast v1.19.2, ast.NullNode is defined as:
type NullNode struct {
*BaseNode
Token *token.Token
}It represents a YAML null scalar node. [1]
🌐 Web query:
goccy go-yaml null node types AST
💡 Result:
In goccy/go-yaml’s AST (github.com/goccy/go-yaml/ast), YAML nulls are represented as:
- Node type enum:
ast.NullType(inast.NodeType’s constants list). [1] - Concrete node:
*ast.NullNodeType()returnsast.NullType[1]GetValue()returns Gonil[1]String()renders asnull[1]
Detecting a null node
import "github.com/goccy/go-yaml/ast"
func isNull(n ast.Node) bool {
if n == nil { return true } // optional, depending on your codepaths
return n.Type() == ast.NullType
// or: _, ok := n.(*ast.NullNode)
}“Empty value” (key:) becomes a NullNode
If you parse YAML like cat: (a mapping key with no value), the value side is a node whose Type() is NullType (i.e., it’s treated as null in the AST). [2]
If you also care about implicit null handling across documents/edge-cases, recent releases mention “Support implicit null”. [3]
Sources: pkg.go.dev AST docs [1], runnable parsing example showing cat: => NullType [2], release notes mentioning implicit null support [3].
🏁 Script executed:
#!/bin/bash
# Check if there are any YAML test files with actual null values
echo "=== Looking for values files with null entries ==="
find . -name "*.yaml" -o -name "*.yml" 2>/dev/null | xargs grep -l ":\s*$\|:\s*null\|:\s*~" 2>/dev/null | grep -v ".git\|node_modules" | head -10
# Look for test data
echo ""
echo "=== Searching for test data files with null values ==="
rg ":\s*null|:\s*~" --type yaml . | head -20Repository: argoproj-labs/argocd-image-updater
Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Verify if NullNode implements the tokenNode interface used in updateNodeToken
# by checking if it has GetToken() and GetComment() methods
echo "=== Check usage patterns of getScalarValue ==="
rg -B 3 -A 3 "getScalarValue\(" pkg/argocd/values_file.go | head -40
echo ""
echo "=== Check the getSetValue/setNodeValue functions ==="
rg -A 10 "func setNodeValue\|func getSetValue" pkg/argocd/values_file.goRepository: argoproj-labs/argocd-image-updater
Length of output: 758
🏁 Script executed:
#!/bin/bash
# Check if there are real-world examples of null values being used
echo "=== Looking at examples or test fixtures ==="
find . -path "./.git" -prune -o -type f \( -name "*.yaml" -o -name "*.yml" \) -print 2>/dev/null | head -5 | xargs cat 2>/dev/null | head -50Repository: argoproj-labs/argocd-image-updater
Length of output: 1406
Add NullNode case to getScalarValue to handle YAML null values.
goccy/go-yaml v1.19.2 represents YAML null values (null, ~, or empty value like key:) as *ast.NullNode. The current implementation of getScalarValue does not handle this type, so accessing a null-valued key will fail with a generic "not a scalar" error. Null values in Helm values files are valid and not uncommon.
Add the following case to getScalarValue (after the AliasNode case):
case *ast.NullNode:
return n.GetToken().Value, nilFor updateNodeToken, the function should already work with NullNode since it implements the required tokenNode interface (has GetToken() and GetComment() methods), but verify this behavior if null values are being modified.
🤖 Prompt for AI Agents
In `@pkg/argocd/values_file.go` around lines 182 - 198, getScalarValue currently
doesn't handle YAML nulls and will return a "not a scalar" error for
*ast.NullNode; add a NullNode branch in getScalarValue (after the *ast.AliasNode
case) to return the node's token value (use n.GetToken().Value) so nulls like
`null`, `~`, or empty `key:` are treated as scalars; also confirm
updateNodeToken works with *ast.NullNode (it should since NullNode implements
GetToken/GetComment) and adjust only if you encounter token interface
mismatches.
3 participants
In this PR, I'm taking another run at the change I tried to make initially in #1166
Currently, when we use Helm values as the write back target, formatting isn't preserved. Given a document like this
Input
and an update where we want to bump the images to
v2.0.0Current update result
after the changes introduced in this PR, the new update would look like this
New update result
Summary by CodeRabbit
New Features
Enhancements
Refactor
Tests