-
Notifications
You must be signed in to change notification settings - Fork 133
direct: Ignore changes between nulls and empty slices/maps #4313
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
Conversation
Empty slices (e.g., `on_failure: []`) and empty maps (e.g., `tags: {}`)
should not cause drift. Added reasons `empty_slice` and `empty_map` to
indicate why these changes are skipped.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
| } else if isEmptySlice(ch.Old, ch.New, ch.Remote) { | ||
| // Empty slice in config should not cause drift when remote has values | ||
| ch.Action = deployplan.Skip | ||
| ch.Reason = deployplan.ReasonEmptySlice |
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.
IIRC, the reason behind this is that nils and empty slices/maps are represented the same in proto.
Can you leave a comment here, or in the isEmptyZZZ functions, explaining the motivation for treating them the same here?
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.
added here 7264c24
|
Commit: 756644d
28 interesting tests: 22 KNOWN, 4 flaky, 1 SKIP, 1 RECOVERED
Top 38 slowest tests (at least 2 minutes):
|
Why
Although these changes can appear in configs, they cannot be represented by SDK's structs, so they cannot be persisted or appear in remote reply, thus causing a permanent drift.
Fixes #4121
Tests
Existing and new acceptance tests.