✨ Initial commit of rollout plugin protobuf #154
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: youngbupark The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a new v1alpha1 rollout protobuf and a go:generate stub. Declares RolloutPluginService RPCs for initialization, rollout/rollback lifecycle, validation, and ManifestWork mutation, plus message types for metadata, cluster rollout status, results, validation payloads, and manifest mutation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/plugin/proto/v1alpha1/gen.go (1)
5-8: Consider adding documentation about additional prerequisites.The installation instructions for the protoc plugins are clear, but consider documenting additional prerequisites:
- The protoc compiler itself must be installed
- Kubernetes dependencies must be vendored (
go mod vendor)- The vendor directory structure must match the import paths
This would help developers successfully generate code on first attempt.
Example enhancement:
// Prerequisites for code generation: // // 1. Install the Protocol Buffers compiler (protoc) // See: https://grpc.io/docs/protoc-installation/ // // 2. Install the protoc-gen-go and protoc-gen-go-grpc plugins // $ go install google.golang.org/protobuf/cmd/protoc-gen-go@latest // $ go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest // // 3. Ensure dependencies are vendored // $ go mod vendor
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pkg/plugin/proto/v1alpha1/rollout.pb.gois excluded by!**/*.pb.gopkg/plugin/proto/v1alpha1/rollout_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
pkg/plugin/proto/v1alpha1/gen.go(1 hunks)pkg/plugin/proto/v1alpha1/rollout.proto(1 hunks)
🧰 Additional context used
🪛 Buf (1.59.0)
pkg/plugin/proto/v1alpha1/rollout.proto
12-12: import "k8s.io/apimachinery/pkg/runtime/generated.proto": file does not exist
(COMPILE)
🔇 Additional comments (5)
pkg/plugin/proto/v1alpha1/rollout.proto (4)
97-113: Verify the field numbering gap in RolloutDetails.Field numbers jump from 4 (
removed) to 6 (placement_total_clusters), skipping field number 5. For a new protobuf definition in an initial commit, this gap is unusual. If field 5 is reserved for future use or was removed during development, consider using thereservedkeyword to make the intent explicit:message RolloutDetails { repeated ClusterRolloutStatus completed = 1; repeated ClusterRolloutStatus in_progress = 2; repeated ClusterRolloutStatus timed_out = 3; repeated ClusterRolloutStatus removed = 4; reserved 5; // Reserved for future use or previously removed field int32 placement_total_clusters = 6; }
130-137: Verify the field numbering gap in RolloutPluginRequest.Field numbers jump from 1 (
metadata) to 7 (rollout_details), skipping field numbers 2-6. For a new protobuf definition in an initial commit, this large gap is unusual and may indicate copy-paste from another message or placeholder for future fields. If these fields are reserved for future use or were removed during development, consider using thereservedkeyword to document the intent:message RolloutPluginRequest { RolloutMeta metadata = 1; reserved 2 to 6; // Reserved for future use optional RolloutDetails rollout_details = 7; }
139-200: Well-designed message definitions.The
ValidateResponse,MutateManifestWorkRequest, andMutateManifestWorkResponsemessages are well-structured with:
- Proper use of proto3
optionalkeyword- Enum values following convention (UNSPECIFIED = 0)
- Flexible response handling with
oneoffor different data types- Clear, descriptive comments
The design supports extensibility while maintaining clarity.
12-12: ****The Kubernetes proto file is correctly imported and available. The file
vendor/k8s.io/apimachinery/pkg/runtime/generated.protoexists in the vendor directory at the exact path specified in the import statement at line 12. Protoc compilation will succeed with this import as-is. No action required.Likely an incorrect or invalid review comment.
pkg/plugin/proto/v1alpha1/gen.go (1)
3-3: No issues found—vendor proto files are properly configured.The verification confirms that:
- The
vendordirectory exists with all required Kubernetes proto filesvendor/k8s.io/apimachinery/pkg/runtime/generated.protois present- The relative path
../../../../vendorin the go:generate directive correctly resolves frompkg/plugin/proto/v1alpha1/- All proto imports in
rollout.protoare satisfiedThe code generation setup is already correct and requires no changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/plugin/proto/v1alpha1/rollout.proto (1)
98-113: Clarify intentional field number gaps withreserveddeclarations.RolloutResult and RolloutPluginRequest have gaps in field numbering (RolloutResult skips field 5, RolloutPluginRequest skips fields 2–6). While proto3 allows this, explicit
reservedstatements improve maintainability and communicate intent to future developers.Apply these diffs to clarify field reservations:
// RolloutResult is the the current result of clusters. message RolloutResult { + reserved 5; // applied is the clusters that have been completed. repeated ClusterRolloutStatus applied = 1;// RolloutPluginRequest is the request to the rollout plugin. message RolloutPluginRequest { + reserved 2, 3, 4, 5, 6; // metadata is the metadata of the rollout. RolloutMeta metadata = 1;Also applies to: 131-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pkg/plugin/proto/v1alpha1/rollout.pb.gois excluded by!**/*.pb.gopkg/plugin/proto/v1alpha1/rollout_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
pkg/plugin/proto/v1alpha1/rollout.proto(1 hunks)
🧰 Additional context used
🪛 Buf (1.59.0)
pkg/plugin/proto/v1alpha1/rollout.proto
12-12: import "k8s.io/apimachinery/pkg/runtime/generated.proto": file does not exist
(COMPILE)
🔇 Additional comments (3)
pkg/plugin/proto/v1alpha1/rollout.proto (3)
16-57: Service and RPC definitions look well-designed.The RolloutPluginService RPC methods are clearly documented with appropriate request/response types. The lifecycle pattern (Begin→Progress→Validate for both rollout and rollback, plus MutateManifestWork) is intuitive.
59-206: Message definitions are well-structured and properly documented.The message types provide clear semantics for request/response payloads, with appropriate use of optional fields, nested enums, and oneof for flexible response data. Naming is consistent and comments are comprehensive.
12-12: ****The import path is correct and the file exists. The file
k8s.io/apimachinery/pkg/runtime/generated.protois vendored at./vendor/k8s.io/apimachinery/pkg/runtime/generated.protoand follows standard Kubernetes proto import conventions. This import will compile successfully with the vendored dependency.Likely an incorrect or invalid review comment.
qiujian16
left a comment
qiujian16
left a comment
There was a problem hiding this comment.
this should be merged after related proposal is merged at first.
something missing in this is we also need a script for generated file. Update via make update and verify with make verify will be even better.
| // MutateManifestWorkResponse is the response to mutate the manifestwork resource before it is applied. | ||
| message MutateManifestWorkResponse { | ||
| // manifestwork is the mutated manifestwork resource. | ||
| k8s.io.apimachinery.pkg.runtime.Unknown manifestwork = 1; |
There was a problem hiding this comment.
the reason that this is unknown is because there is no proto definition for manifestwork?
2 participants
Summary
This is the initial commit of rollout plugin protobuf for the enhancement open-cluster-management-io/enhancements#160
Related issue(s)
Fixes #
Summary by CodeRabbit