-
Notifications
You must be signed in to change notification settings - Fork 37
feat(rbg): support role-level metadata #113
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
feat(rbg): support role-level metadata #113
Conversation
Summary of ChangesHello @Syspretor, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant feature that allows for the specification of custom labels and annotations directly within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces support for role-level metadata (labels and annotations) in RoleBasedGroup resources. The changes are well-structured, touching the API types, generated client code, CRD definitions, and reconcilers. The reconcilers for Deployment, LeaderWorkerSet, and StatefulSet are correctly updated to merge the new role-level metadata with system-generated metadata. New tests have been added to cover this new functionality, which is great. I have a few suggestions to improve the CRD documentation and strengthen the new tests.
deploy/helm/rbgs/crds/workloads.x-k8s.io_rolebasedgroupsets.yaml
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cdb4ef0 to
b0b4f7d
Compare
b0b4f7d to
27e1622
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a valuable feature by adding support for role-level labels and annotations, which will be propagated to the workloads. The implementation is mostly correct and is accompanied by thorough tests for the new functionality.
However, I've found a couple of critical compilation errors in deploy_reconciler.go and sts_reconciler.go due to a missing maps package import. These need to be fixed.
Additionally, I've identified some medium-severity issues:
- There is significant code duplication across the new test files. I've suggested refactoring this into a shared helper to improve maintainability.
- The CRD and Helm chart YAML files contain truncated URLs in the descriptions for labels, which should be corrected for better documentation.
Once these issues are addressed, the pull request should be in good shape.
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.
Pull request overview
This PR adds support for role-level labels and annotations in RoleBasedGroup resources. These metadata fields can be configured at the role level and are automatically propagated to the labels and annotations of workloads (StatefulSet, Deployment, LeaderWorkerSet) created from the role definition. The implementation ensures system-generated labels always take precedence over user-provided values.
Key changes:
- Added
LabelsandAnnotationsfields to theRoleSpecAPI type - Updated all three workload reconcilers to merge role-level metadata with system-generated metadata
- Added comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| api/workloads/v1alpha1/rolebasedgroup_types.go | Added Labels and Annotations fields to RoleSpec with proper documentation |
| api/workloads/v1alpha1/zz_generated.deepcopy.go | Auto-generated deep copy methods for new map fields |
| client-go/applyconfiguration/workloads/v1alpha1/rolespec.go | Auto-generated apply configuration helpers for Labels and Annotations |
| config/crd/bases/workloads.x-k8s.io_rolebasedgroups.yaml | CRD schema updated with Labels and Annotations field definitions |
| config/crd/bases/workloads.x-k8s.io_rolebasedgroupsets.yaml | CRD schema updated with Labels and Annotations field definitions |
| deploy/helm/rbgs/crds/workloads.x-k8s.io_rolebasedgroups.yaml | Helm CRD updated with Labels and Annotations field definitions |
| deploy/helm/rbgs/crds/workloads.x-k8s.io_rolebasedgroupsets.yaml | Helm CRD updated with Labels and Annotations field definitions |
| pkg/reconciler/sts_reconciler.go | StatefulSet reconciler updated to merge role labels/annotations using labels.Merge |
| pkg/reconciler/lws_reconciler.go | LeaderWorkerSet reconciler updated to merge role labels/annotations using labels.Merge |
| pkg/reconciler/deploy_reconciler.go | Deployment reconciler updated to merge role labels/annotations using labels.Merge |
| pkg/reconciler/sts_reconciler_test.go | Added comprehensive tests for label/annotation merging with priority verification |
| pkg/reconciler/lws_reconciler_test.go | Added comprehensive tests for label/annotation merging with priority verification |
| pkg/reconciler/deploy_reconciler_test.go | Added comprehensive tests for label/annotation merging with priority verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cheyang
left a comment
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.
/lgtm
/approve
Ⅰ. Motivation
Support configuring Role-level labels/annotations; these settings will be propagated to the labels/annotations of workloads created from the Role definition.
Ⅱ. Modifications
Introducing new API Fields under
rbg.spec.roleⅢ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
# kubectl get sts demo-engine -o jsonpath="{.metadata.labels['custom-label-key']}{'\n'}" custome-label-valueVI. Special notes for reviews
Checklist
make fmt.