render: Replace condition timestamps and sort resources to stabilize output#82
render: Replace condition timestamps and sort resources to stabilize output#82adamwg wants to merge 2 commits into
Conversation
The old render command used a static timestamp for status condition `lastTransitionTime`s so that output was stable across runs with the same inputs. The new render engine calls the real reconciler, and therefore produces real timestamps. Restore stability by replacing the timestamps in any status conditions returned by the render engine before outputting them. We use the same static timestamp that was present in the old CLI for consistency with its output. Fixes crossplane#50 Signed-off-by: Adam Wolfe Gordon <[email protected]>
…tion The old CLI's `render` command sorted composed resources by the `composition-resource-name` annotation to ensure output consistency across runs with the same inputs. The real reconciler code used by the new render engine does not sort the results, so we need to sort them client-side to ensure stability. Fixes crossplane#53 Signed-off-by: Adam Wolfe Gordon <[email protected]>
|
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented. 📝 WalkthroughWalkthroughThis PR resolves non-determinism in ChangesRender Output Determinism
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/crossplane/render/xr/cmd.go (1)
345-357: ⚡ Quick winThe sorting logic looks great! The use of
slices.SortStableFuncensures deterministic ordering across runs, and the nil-checks with empty-string defaults are robust.One small consistency observation: the sorting logic (lines 350, 353) uses
xcrd.AnnotationKeyCompositionResourceName, but line 367 below usesrender.AnnotationKeyCompositionResourceNamein the error message. While both likely refer to the same annotation key string, it would be more maintainable to use the same constant reference throughout this file. Which constant do you prefer to standardize on?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/render/xr/cmd.go` around lines 345 - 357, The file mixes two constants for the same annotation key—xcrd.AnnotationKeyCompositionResourceName and render.AnnotationKeyCompositionResourceName—causing inconsistency; pick one constant and replace the other so the code uses a single canonical symbol throughout (e.g., update the sorting block or the error message to consistently reference either xcrd.AnnotationKeyCompositionResourceName or render.AnnotationKeyCompositionResourceName), ensuring all uses (including in slices.SortStableFunc over out.ComposedResources and any error messages) refer to the chosen constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/crossplane/render/xr/cmd.go`:
- Around line 345-357: The file mixes two constants for the same annotation
key—xcrd.AnnotationKeyCompositionResourceName and
render.AnnotationKeyCompositionResourceName—causing inconsistency; pick one
constant and replace the other so the code uses a single canonical symbol
throughout (e.g., update the sorting block or the error message to consistently
reference either xcrd.AnnotationKeyCompositionResourceName or
render.AnnotationKeyCompositionResourceName), ensuring all uses (including in
slices.SortStableFunc over out.ComposedResources and any error messages) refer
to the chosen constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19b56d26-f8b8-4e7e-aa9e-c894ec852794
📒 Files selected for processing (4)
cmd/crossplane/render/op/cmd.gocmd/crossplane/render/output.gocmd/crossplane/render/output_test.gocmd/crossplane/render/xr/cmd.go
|
@adamwg for sorting this is related to: crossplane/crossplane#7341 |
|
Thank you @adamwg for these fixes!
Yes, it is and the behaviour appears when using the CLI, but compared to the changes in this PR, it should not be fixed client side in the CLI |
3 participants
Description of your changes
The old render command used a static timestamp for status condition
lastTransitionTimes so that output was stable across runs with the same inputs. The new render engine calls the real reconciler, and therefore produces real timestamps.Restore stability by replacing the timestamps in any status conditions returned by the render engine before outputting them. We use the same static timestamp that was present in the old CLI for consistency with its output.
Fixes #50
The old CLI's
rendercommand sorted composed resources by thecomposition-resource-nameannotation to ensure output consistency across runs with the same inputs. The real reconciler code used by the new render engine does not sort the results, so we need to sort them client-side to ensure stability.Fixes #53
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Linked a PR or a docs tracking issue to document this change.backport release-x.ylabels to auto-backport this PR.