refactor(pipelineinspector): use oneof for operation/composition context#914
Conversation
0d285d8 to
62624b0
Compare
Extract composition and operation metadata into separate message types and use a oneof to enforce that only one context type is set per step. This allows the PipelineInspector to handle both Composition-based pipelines and Operation-based pipelines with a clear distinction at the protocol level. Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
62624b0 to
4f288e3
Compare
📝 WalkthroughWalkthroughRestructures StepMeta in the Pipeline Inspector proto: adds google.protobuf.Timestamp and a oneof context holding new OperationMeta or CompositionMeta messages; moves previous composition-related fields into CompositionMeta and renumbers several StepMeta fields. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Thank you — would you like me to generate a protobuf migration note or compatibility checklist for consumers of StepMeta? 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apis/pipelineinspector/proto/v1alpha1/pipeline_inspector.proto`:
- Around line 92-100: The new oneof "context" currently reuses field numbers 7
and 8 for OperationMeta and CompositionMeta which can break forward/backward
compatibility; instead add a reserved range for the previously-used field
numbers (e.g., reserve 7 to 12) and move the oneof entries to unused, higher
field numbers (choose numbers not overlapping timestamp = 13), updating the
oneof declaration (context) to use the new numbers for OperationMeta and
CompositionMeta, then regenerate stubs; ensure you reference the symbols
OperationMeta, CompositionMeta, context (oneof) and timestamp when making the
change so the proto evolution is explicit and old field numbers remain reserved.
jbw976
left a comment
There was a problem hiding this comment.
This looks correct according to the protobuf documentation on https://protobuf.dev/programming-guides/proto3/#oneof, and the generated go code looks reasonable as well ✅
| message CompositionMeta { | ||
| // Name of the Composition defining this pipeline. | ||
| string composition_name = 7; | ||
| string composition_name = 1; |
There was a problem hiding this comment.
would composition revision be useful too?
There was a problem hiding this comment.
The composition revision is available through the composite at spec.crossplane.compositionRevisionRef
Extract composition and operation metadata into separate message types and use a oneof to enforce that only one context type is set per step.
This allows the PipelineInspector to handle both Composition-based pipelines and Operation-based pipelines with a clear distinction at the protocol level.
Description of your changes
Yet another breaking change to the inspector interface to account for Operations too, see crossplane/crossplane#7025 (comment).
Fixes #
I have:
earthly +reviewableto ensure this PR is ready for review.Added or updated unit tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.