Fix yaml formatting in config-remote-sync #4468
Open
+114
−9
Conversation
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
ilyakuz-db
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
commented
Feb 9, 2026
| + interval: 2 | ||
| unit: DAYS | ||
| - unit: DAYS | ||
| + pause_status: UNPAUSED |
Contributor
Author
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-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
reviewed
Feb 9, 2026
| 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?
Contributor
Author
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.
Contributor
Author
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-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 3e07780
26 interesting tests: 11 RECOVERED, 8 flaky, 5 SKIP, 1 KNOWN, 1 FAIL
Top 50 slowest tests (at least 2 minutes):
|
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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