MGMT-22992: Add role and role binding reconciler skeletons - #486
Conversation
|
@jhernand: This pull request references MGMT-22992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (10)
WalkthroughThis pull request adds support for two new Kubernetes reconcilers—Role and RoleBinding—to the controller service. The changes include implementing two reconciler functions with a FunctionBuilder pattern, wiring them into the controller startup flow, extending protobuf event message definitions to support Role and RoleBinding payloads in both public and private event types, and updating server-side payload handling to process the new event object types in both the generic and events servers. Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controllers/role/role_reconciler_function.go (1)
64-70: 💤 Low valueConsider explicitly capturing the result of
masks.NewCalculator().Build()Although
Build()currently returns only*Calculator, inlining it into the struct literal means any future error return would be silently dropped. Extracting it would be safer for future maintenance:maskCalculator, err := masks.NewCalculator().Build() if err != nil { return nil, err } result = &function{ logger: b.logger, rolesClient: privatev1.NewRolesClient(b.connection), maskCalculator: maskCalculator, }🤖 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 `@internal/controllers/role/role_reconciler_function.go` around lines 64 - 70, Extract the call to masks.NewCalculator().Build() into a local variable (e.g., maskCalculator, err := masks.NewCalculator().Build()), check and return the error if non-nil, and then set maskCalculator in the &function{...} struct instead of inlining the call; update the surrounding constructor that returns result to propagate the error (return nil, err) when Build() fails so future changes to Build() won't silently drop errors.
🤖 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 `@internal/controllers/role/role_reconciler_function.go`:
- Around line 64-70: Extract the call to masks.NewCalculator().Build() into a
local variable (e.g., maskCalculator, err := masks.NewCalculator().Build()),
check and return the error if non-nil, and then set maskCalculator in the
&function{...} struct instead of inlining the call; update the surrounding
constructor that returns result to propagate the error (return nil, err) when
Build() fails so future changes to Build() won't silently drop errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c75454fb-5019-49ac-b03e-2b4418491185
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
internal/cmd/service/start/controller/start_controller_cmd.gointernal/controllers/role/role_reconciler_function.gointernal/controllers/rolebinding/role_binding_reconciler_function.gointernal/servers/events_server.gointernal/servers/generic_server.goproto/private/osac/private/v1/event_type.protoproto/public/osac/public/v1/event_type.proto
Add `role` and `role_binding` payloads to both the private and public `Event` messages so the reconciler watch and list sync mechanisms work correctly. Register both types in `GenericServer.setPayload` for notify and add the corresponding `extractMetadata` cases in the public events server. Wire the reconcilers in `start_controller_cmd`. For now these are only skeletons that add the controller finalizer, set default status, and log the object being reconciled. The actual reconciliation logic will be added in later patches. Related: https://redhat.atlassian.net/browse/MGMT-22992 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
fd1cd8c to
7128249
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary
roleandrole_bindingpayloads to both the private and publicEventmessages so that the reconciler watch/LIST sync mechanisms and public event streaming work correctly.GenericServer.setPayloadfor NOTIFY and add correspondingextractMetadatacases in the public events server.start_controller_cmdthat add the controller finalizer, set default status, and log the reconciled object. The actual reconciliation logic will be added in later patches.Test plan
buf lintpasses.go build ./...succeeds.ginkgo run -r internal).Related: https://redhat.atlassian.net/browse/MGMT-22992
Summary by CodeRabbit