Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 214
Fix yaml formatting in config-remote-sync#4468
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
dd609e4479a722e0bcfc6d05c9b81d95659ad2bc943e077805ea553c9080fd5File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -14,7 +14,9 @@ Resource: resources.jobs.my_job | ||
| email_notifications.on_failure: add | ||
| parameters: replace | ||
| tags['team']: add | ||
| trigger.periodic.interval: replace | ||
| trigger.pause_status: add | ||
| trigger.periodic: remove | ||
| trigger.table_update: add | ||
| @@ -29,7 +31,7 @@ Resource: resources.jobs.my_job | ||
| - | ||
| resources: | ||
| jobs: | ||
| @@ -8,12 +7,17 @@ | ||
| @@ -8,13 +7,19 @@ | ||
| on_success: | ||
| - success@example.com | ||
| + no_alert_for_skipped_runs: true | ||
| @@ -47,12 +49,16 @@ Resource: resources.jobs.my_job | ||
| + - default: us-east-1 | ||
| + name: region | ||
| trigger: | ||
| periodic: | ||
| - periodic: | ||
| - interval: 1 | ||
| + interval: 2 | ||
| unit: DAYS | ||
| - unit: DAYS | ||
| + pause_status: UNPAUSED | ||
| ||
| + table_update: | ||
| + table_names: | ||
| + - samples.nyctaxi.trips | ||
| environments: | ||
| @@ -31,5 +35,6 @@ | ||
| - environment_key: default | ||
| @@ -31,5 +36,6 @@ | ||
| node_type_id: [NODE_TYPE_ID] | ||
| num_workers: 1 | ||
| - | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package configsync | ||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| @@ -14,12 +15,14 @@ import ( | ||
| "github.com/databricks/cli/libs/structs/structpath" | ||
| "github.com/palantir/pkg/yamlpatch/gopkgv3yamlpatcher" | ||
| "github.com/palantir/pkg/yamlpatch/yamlpatch" | ||
| "go.yaml.in/yaml/v3" | ||
| ) | ||
| // ApplyChangesToYAML generates YAML files for the given field changes. | ||
| func ApplyChangesToYAML(ctx context.Context, b *bundle.Bundle, fieldChanges []FieldChange) ([]FileChange, error) { | ||
| originalFiles := make(map[string][]byte) | ||
| modifiedFiles := make(map[string][]byte) | ||
| fileFieldChanges := make(map[string][]FieldChange) | ||
| for _, fieldChange := range fieldChanges { | ||
| filePath := fieldChange.FilePath | ||
| @@ -39,14 +42,22 @@ func ApplyChangesToYAML(ctx context.Context, b *bundle.Bundle, fieldChanges []Fi | ||
| } | ||
| modifiedFiles[filePath] = modifiedContent | ||
| fileFieldChanges[filePath] = append(fileFieldChanges[filePath], fieldChange) | ||
| } | ||
| var result []FileChange | ||
| for filePath := range modifiedFiles { | ||
| // TODO: A good alternative approach is to remove parent nodes during the Resolve phase, | ||
| // when all of their keys/items are removed, but this should be tested for edge cases. | ||
| // In this case flow style will never appear because empty nodes are never serialized and we won't need clearAddedFlowStyle | ||
| normalized, err := clearAddedFlowStyle(modifiedFiles[filePath], fileFieldChanges[filePath]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to normalize YAML style in %s: %w", filePath, err) | ||
| } | ||
| result = append(result, FileChange{ | ||
| Path: filePath, | ||
| OriginalContent: string(originalFiles[filePath]), | ||
| ModifiedContent: string(modifiedFiles[filePath]), | ||
| ModifiedContent: string(normalized), | ||
| }) | ||
| } | ||
| @@ -262,3 +273,81 @@ func strPathToJSONPointer(pathStr string) (string, error) { | ||
| } | ||
| return "/" + strings.Join(parts, "/"), nil | ||
| } | ||
| // clearAddedFlowStyle clears FlowStyle on YAML nodes along the changed field paths. | ||
| // This prevents flow-style formatting (e.g. {key: value}) that yaml.v3 introduces | ||
| // when empty mappings are serialized as "{}" during patch operations | ||
| func clearAddedFlowStyle(content []byte, fieldChanges []FieldChange) ([]byte, error) { | ||
| var doc yaml.Node | ||
| if err := yaml.Unmarshal(content, &doc); err != nil { | ||
| return content, nil | ||
| } | ||
| for _, fc := range fieldChanges { | ||
| for _, candidate := range fc.FieldCandidates { | ||
| clearFlowStyleAlongPath(&doc, candidate) | ||
| } | ||
| } | ||
| var buf bytes.Buffer | ||
| enc := yaml.NewEncoder(&buf) | ||
| enc.SetIndent(2) | ||
| if err := enc.Encode(&doc); err != nil { | ||
| return nil, err | ||
| } | ||
| return buf.Bytes(), enc.Close() | ||
| } | ||
| // clearFlowStyleAlongPath navigates the YAML tree along the given structpath, | ||
| // clearing FlowStyle on every node from root to leaf (inclusive). | ||
| func clearFlowStyleAlongPath(doc *yaml.Node, pathStr string) { | ||
| node, err := structpath.ParsePath(pathStr) | ||
| if err != nil { | ||
| return | ||
| } | ||
| current := doc | ||
| if current.Kind == yaml.DocumentNode && len(current.Content) > 0 { | ||
| current = current.Content[0] | ||
| } | ||
| for _, n := range node.AsSlice() { | ||
| current.Style &^= yaml.FlowStyle | ||
| if key, ok := n.StringKey(); ok { | ||
| if current.Kind != yaml.MappingNode { | ||
| return | ||
| } | ||
| found := false | ||
| // current.Content: [key1, val1, key2, val2, ...] | ||
| for i := 0; i+1 < len(current.Content); i += 2 { | ||
| if current.Content[i].Value == key { | ||
| current = current.Content[i+1] | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| return | ||
| } | ||
| continue | ||
| } | ||
| if idx, ok := n.Index(); ok { | ||
| if current.Kind != yaml.SequenceNode || idx < 0 || idx >= len(current.Content) { | ||
| return | ||
| } | ||
| current = current.Content[idx] | ||
| continue | ||
| } | ||
| return | ||
| } | ||
| clearFlowStyleNodes(current) | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth having a This function looks generalizable. OK if not worth it right now. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not worth it, I'd like this to be only a temporary solution. I think a better fix will be to do the following (from the PR description):
For now, I wanted to quickly fix it now to avoid weird diff entries, but later I think it makes sense to investigate it | ||
| } | ||
| func clearFlowStyleNodes(node *yaml.Node) { | ||
| node.Style &^= yaml.FlowStyle | ||
| for _, child := range node.Content { | ||
| clearFlowStyleNodes(child) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are omitted from the output, expected?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We show here not the full config but the diff between old config and new. So it is just a confusing snippet from the unified diff output
In reality, these lines are unchanged, but there is an empty-line deletion on line 42 (separate issue, will be addressed later), and this
# Flow-style formatting (should be preserved)part is just brought here as viewing context