Skip to content
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

feat: preserve formatting and comments when updating Helm values file #1039

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: preserve formatting and comments when updating Helm values file #1039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
github.com/bradleyfalzon/ghinstallation/v2 v2.12.0
github.com/distribution/distribution/v3 v3.0.0-20230722181636-7b502560cad4
github.com/go-git/go-git/v5 v5.13.2
github.com/goccy/go-yaml v1.15.22
github.com/google/uuid v1.6.0
github.com/patrickmn/go-cache v2.1.0+incompatible
github.com/prometheus/client_golang v1.20.5
Expand All @@ -26,7 +27,6 @@ require (
golang.org/x/oauth2 v0.25.0
golang.org/x/sync v0.10.0
google.golang.org/grpc v1.70.0
gopkg.in/yaml.v2 v2.4.0
k8s.io/api v0.31.0
k8s.io/apimachinery v0.31.0
k8s.io/client-go v0.31.0
Expand Down Expand Up @@ -154,6 +154,7 @@ require (
gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/warnings.v0 v0.1.2 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/apiextensions-apiserver v0.31.2 // indirect
k8s.io/apiserver v0.31.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJA
github.com/gobwas/httphead v0.1.0/go.mod h1:O/RXo79gxV8G+RqlR/otEwx4Q36zl9rqC5u12GKvMCM=
github.com/gobwas/pool v0.2.1/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6WezmKEw=
github.com/gobwas/ws v1.2.1/go.mod h1:hRKAFb8wOxFROYNsT1bqfWnhX+b5MFeJM9r2ZSwg/KY=
github.com/goccy/go-yaml v1.15.22 h1:iQI1hvCoiYYiVFq76P4AI8ImgDOfgiyKnl/AWjK8/gA=
github.com/goccy/go-yaml v1.15.22/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA=
github.com/gogits/go-gogs-client v0.0.0-20200905025246-8bb8a50cb355 h1:HTVNOdTWO/gHYeFnr/HwpYwY6tgMcYd+Rgf1XrHnORY=
github.com/gogits/go-gogs-client v0.0.0-20200905025246-8bb8a50cb355/go.mod h1:cY2AIrMgHm6oOHmR7jY+9TtjzSjQ3iG7tURJG3Y6XH0=
github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
Expand Down
2 changes: 1 addition & 1 deletion pkg/argocd/argocd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ func getHelmParamNamesFromAnnotation(annotations map[string]string, img *image.C
// Get a named helm parameter from a list of parameters
func getHelmParam(params []v1alpha1.HelmParameter, name string) *v1alpha1.HelmParameter {
for _, param := range params {
if param.Name == name {
if param.Name == strings.Trim(name, "'\"") {
return &param
}
}
Expand Down
166 changes: 106 additions & 60 deletions pkg/argocd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"path/filepath"
"regexp"
"strings"
"sync"
"text/template"
Expand All @@ -21,7 +22,11 @@ import (

"github.com/argoproj/argo-cd/v2/pkg/apiclient/application"
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"gopkg.in/yaml.v2"

"github.com/goccy/go-yaml"
"github.com/goccy/go-yaml/ast"
"github.com/goccy/go-yaml/parser"
"github.com/goccy/go-yaml/token"
)

// Stores some statistics about the results of a run
Expand Down Expand Up @@ -459,8 +464,7 @@ func marshalParamsOverride(app *v1alpha1.Application, originalData []byte) ([]by
if strings.HasPrefix(app.Annotations[common.WriteBackTargetAnnotation], common.HelmPrefix) {
images := GetImagesAndAliasesFromApplication(app)

helmNewValues := yaml.MapSlice{}
err = yaml.Unmarshal(originalData, &helmNewValues)
helmNewValues, err := parser.ParseBytes(originalData, parser.ParseComments)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -488,7 +492,7 @@ func marshalParamsOverride(app *v1alpha1.Application, originalData []byte) ([]by
if helmParamVersion == nil {
return nil, fmt.Errorf("%s parameter not found", helmAnnotationParamVersion)
}
err = setHelmValue(&helmNewValues, helmAnnotationParamVersion, helmParamVersion.Value)
err = setHelmValue(helmNewValues, helmAnnotationParamVersion, helmParamVersion.Value)
if err != nil {
return nil, fmt.Errorf("failed to set image parameter version value: %v", err)
}
Expand All @@ -499,13 +503,13 @@ func marshalParamsOverride(app *v1alpha1.Application, originalData []byte) ([]by
return nil, fmt.Errorf("%s parameter not found", helmAnnotationParamName)
}

err = setHelmValue(&helmNewValues, helmAnnotationParamName, helmParamName.Value)
err = setHelmValue(helmNewValues, helmAnnotationParamName, helmParamName.Value)
if err != nil {
return nil, fmt.Errorf("failed to set image parameter name value: %v", err)
}
}

override, err = yaml.Marshal(helmNewValues)
override = []byte(helmNewValues.String())
} else {
var params helmOverride
newParams := helmOverride{
Expand Down Expand Up @@ -561,76 +565,118 @@ func mergeKustomizeOverride(t *kustomizeOverride, o *kustomizeOverride) {
}
}

// Check if a key exists in a MapSlice and return its index and value
func findHelmValuesKey(m yaml.MapSlice, key string) (int, bool) {
for i, item := range m {
if item.Key == key {
return i, true
func splitKeys(input string) []string {
// Regular expression to match quoted and unquoted segments
re := regexp.MustCompile(`'([^']*)'|"([^"]*)"|([^.".]+)`)
matches := re.FindAllStringSubmatch(input, -1)

var result []string
for _, match := range matches {
if match[1] != "" { // Single-quoted string
result = append(result, match[1])
} else if match[2] != "" { // Double-quoted string
result = append(result, match[2])
} else if match[3] != "" { // Unquoted segment
result = append(result, match[3])
}
}
return -1, false

return result
}

// set value of the parameter passed from the annotations.
func setHelmValue(currentValues *yaml.MapSlice, key string, value interface{}) error {
// Check if the full key exists
if idx, found := findHelmValuesKey(*currentValues, key); found {
(*currentValues)[idx].Value = value
return nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

the current behaviro is, if the full key (e.g., images.nginx.name) already exists in the target helm file, take this existing key and update its value. Otherwise, assume the users want nested parameter names. I think this should be perserved when the annotation value helm.image-name and helm.image-tag are not explicitly quoted.

Even when the 2 annotation values are explicitly quoted, it's still worth considering whether we should honor the existing key format (flat dotted or nested) over the application annotation.

func setHelmValue(file *ast.File, keyPath, value string) error {
path := splitKeys(keyPath)
if len(path) == 0 {
return fmt.Errorf("empty key provided")
}

var err error
keys := strings.Split(key, ".")
current := currentValues
var parent *yaml.MapSlice
parentIdx := -1

for i, k := range keys {
if idx, found := findHelmValuesKey(*current, k); found {
if i == len(keys)-1 {
// If we're at the final key, set the value and return
(*current)[idx].Value = value
return nil
} else {
// Navigate deeper into the map
if nestedMap, ok := (*current)[idx].Value.(yaml.MapSlice); ok {
parent = current
parentIdx = idx
current = &nestedMap
} else {
return fmt.Errorf("unexpected type %T for key %s", (*current)[idx].Value, k)
var mapping *ast.MappingNode
if file.Docs[0].Body == nil {
tk := token.New("$", "$", &token.Position{})
mapping = ast.Mapping(tk, false)
file.Docs[0].Body = mapping
} else {
mapping, _ = file.Docs[0].Body.(*ast.MappingNode)
if mapping == nil {
return fmt.Errorf("yaml is invalid")
}
}

// Traverse the path
var lastNode *ast.MappingValueNode
for index, key := range path {
found := false
var currentNode *ast.MappingValueNode

for _, v := range mapping.Values {
if v.Key.GetToken().Value == key {
currentNode = v
if index == len(path)-1 {
lastNode = currentNode
found = true
break
}
// Move deeper into the structure
if nextMapping, ok := v.Value.(*ast.MappingNode); ok {
mapping = nextMapping
found = true
break
}
}
} else {
newCurrent := yaml.MapSlice{}
var newParent yaml.MapSlice
}

if i == len(keys)-1 {
newParent = append(*current, yaml.MapItem{Key: k, Value: value})
} else {
newParent = append(*current, yaml.MapItem{Key: k, Value: newCurrent})
}
// If key does not exist, create it
if !found {
// Create a token with proper position (assuming default line/column)
keyToken := token.New(key, key, &token.Position{Column: index*2 + 1})
newKey := ast.String(keyToken) // Create key node
mappingToken := token.New(key, key, &token.Position{})
newMapping := ast.Mapping(mappingToken, false) // Create empty mapping

if parent == nil {
*currentValues = newParent
} else {
// if parentIdx has not been set (parent element is also new), set it to the last element
if parentIdx == -1 {
parentIdx = len(*parent) - 1
if parentIdx < 0 {
parentIdx = 0
}
if currentNode != nil {
comment := currentNode.Value.GetComment()
currentNode.Value = newMapping
err := currentNode.Value.SetComment(comment)
if err != nil {
return err
}
(*parent)[parentIdx].Value = newParent
lastNode = currentNode
} else {
// Add the new mapping to the parent mapping
lastNode = ast.MappingValue(mappingToken, newKey, newMapping)
mapping.Values = append(mapping.Values, lastNode)
}

parent = &newParent
current = &newCurrent
parentIdx = -1
mapping = newMapping
}
}

return err
if lastNode == nil {
return fmt.Errorf("key not found")
}

var valueToken *token.Token
if token.IsNeedQuoted(value) {
valueToken = token.SingleQuote(value, value, &token.Position{})
} else {
valueToken = token.New(value, value, &token.Position{})
}
newValue := ast.String(valueToken)
comment := lastNode.Value.GetComment()
if comment == nil {
comment = lastNode.Key.GetComment()
}
lastNode.Value = newValue
err := lastNode.Key.SetComment(nil)
if err != nil {
return err
}
err = lastNode.Value.SetComment(comment)
if err != nil {
return err
}

return nil
}

func getWriteBackConfig(app *v1alpha1.Application, kubeClient *kube.ImageUpdaterKubernetesClient, argoClient ArgoCD) (*WriteBackConfig, error) {
Expand Down
Loading
Loading