Uh oh!
There was an error while loading. Please reload this page.
Fix yaml formatting in config-remote-sync - #4468
Merged
Merged
Conversation
ilyakuz-db
requested review from
andrewnester, anton-107, denik, pietern and shreyas-goenka
as code ownersFebruary 9, 2026 11:47
ilyakuz-dbtemporarily deployed
to
test-trigger-is
February 9, 2026 11:47 — with 
GitHub Actions
Inactive
ilyakuz-db
commented
Feb 9, 2026
| + interval: 2 | ||
| unit: DAYS | ||
| - unit: DAYS | ||
| + pause_status: UNPAUSED |
ContributorAuthor
There was a problem hiding this comment.
This case was previously incorrectly formatted in Flow style, I updated the test to capture this (see test behavior before the fix dd609e4)
Collaborator
Commit: d05c9b8
17 interesting tests: 7 KNOWN, 7 SKIP, 2 flaky, 1 RECOVERED
Top 50 slowest tests (at least 2 minutes):
|
ilyakuz-dbtemporarily deployed
to
test-trigger-is
February 9, 2026 13:13 — with GitHub Actions
Inactive
pietern
reviewed
Feb 9, 2026
| require github.com/google/jsonschema-go v0.4.2 // MIT | ||
| require gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| require gopkg.in/yaml.v3 v3.0.1 |
Contributor
There was a problem hiding this comment.
Can you use go.yaml.in/yaml/v3 instead?
See #4353.
| team: data-eng | ||
| - | ||
| # Flow-style formatting (should be preserved) | ||
| parameters: |
Contributor
There was a problem hiding this comment.
They are omitted from the output, expected?
ContributorAuthor
There was a problem hiding this comment.
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
| return | ||
| } | ||
| clearFlowStyleNodes(current) |
Contributor
There was a problem hiding this comment.
Is it worth having a yaml.Node walker based on structpath?
This function looks generalizable.
OK if not worth it right now.
ContributorAuthor
There was a problem hiding this comment.
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):
TODO: A good alternative is to remove parent nodes during the Resolve phase, when all of their keys/items are removed, but this is a separate task and needs to be tested for edge cases. I'll clean up this fix if the parent removal is a valid option
For now, I wanted to quickly fix it now to avoid weird diff entries, but later I think it makes sense to investigate it
ilyakuz-dbtemporarily deployed
to
test-trigger-is
February 9, 2026 15:09 — with GitHub Actions
Inactive
Collaborator
Commit: 9080fd5
20 interesting tests: 8 KNOWN, 5 SKIP, 4 RECOVERED, 3 flaky
Top 50 slowest tests (at least 2 minutes):
|
ilyakuz-dbtemporarily deployed
to
test-trigger-is
February 10, 2026 07:46 — with GitHub Actions
Inactive
ilyakuz-dbtemporarily deployed
to
test-trigger-is
February 10, 2026 22:16 — with GitHub Actions
Inactive
ilyakuz-dbtemporarily deployed
to
test-trigger-is
February 10, 2026 22:38 — with GitHub Actions
Inactive
denik
approved these changes
Feb 12, 2026
denik
left a comment
Contributor
There was a problem hiding this comment.
nit: It would be best to copy TODO in the code as well and not just in PR desc
ilyakuz-db
commented
Feb 12, 2026
ContributorAuthor
Good idea, added |
ilyakuz-dbtemporarily deployed
to
test-trigger-is
February 12, 2026 15:13 — with GitHub Actions
Inactive
Uh oh!
There was an error while loading. Please reload this page.
denik pushed a commit
that referenced
this pull request
May 20, 2026
## Changes Fix cases where the config-remote-sync command formats nodes in [Flow style](https://www.yaml.info/learn/flowstyle.html) ## Why It happens when a neighbor node is removed during sync, and new fields are then added to the same parent. Empty nodes can't be expressed in block style, therefore, yaml patcher falls back to using Flow formatting, which is then inherited by created child nodes TODO: A good alternative is to remove parent nodes during the Resolve phase, when all of their keys/items are removed, but this is a separate task and needs to be tested for edge cases. I'll clean up this fix if the parent removal is a valid option ## Tests Added acceptance tests <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Changes
Fix cases where the config-remote-sync command formats nodes in Flow style
Why
It happens when a neighbor node is removed during sync, and new fields are then added to the same parent. Empty nodes can't be expressed in block style, therefore, yaml patcher falls back to using Flow formatting, which is then inherited by created child nodes
TODO: A good alternative is to remove parent nodes during the Resolve phase, when all of their keys/items are removed, but this is a separate task and needs to be tested for edge cases. I'll clean up this fix if the parent removal is a valid option
Tests
Added acceptance tests