Repository navigation
[RHAISTRAT-1064] allow specifying submodules, and mirror status into DSC - #3838
openshift-merge-bot[bot] merged 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DSC status types for AIGateway submodules, release metadata extraction and propagation, and reflection-based component status updates. Module reconciliation now mirrors submodule conditions, handles disabled, stale, and error states, and updates submodule management states. AIGateway configuration defines ModelsAsAService and BatchGateway mappings. Unit and end-to-end tests cover release mirroring, condition propagation, enablement transitions, and fallback behavior. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
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: 1
🧹 Nitpick comments (1)
internal/controller/modules/base.go (1)
183-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo identical reflection-based management-state writers. Both resolve a field on
dsc.Status.Componentsby name and set itsManagementStatetooperatorv1.Managed/Removed; the only difference is the field-name source. Extract a shared helper (e.g.setComponentManagementState(components any, fieldName string, enabled bool)) and call it from both, so the reflection/CanSetlogic lives in one place.
internal/controller/modules/base.go#L183-L198: haveWriteDSCComponentStatusdelegate to the shared helper usingb.Config.GVK.Kind.internal/controller/modules/modules_controller_actions.go#L692-L711: havewriteSubmoduleComponentStatusdelegate to the same helper usingsm.StatusFieldName(keep theplatformCtx.DSC == nil/empty-name guards).🤖 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/controller/modules/base.go` around lines 183 - 198, Extract the duplicated reflection and ManagementState assignment into a shared setComponentManagementState helper. In internal/controller/modules/base.go:183-198, update BaseHandler.WriteDSCComponentStatus to call it with b.Config.GVK.Kind; in internal/controller/modules/modules_controller_actions.go:692-711, update writeSubmoduleComponentStatus to call it with sm.StatusFieldName while preserving the existing platformCtx.DSC nil and empty-name guards.
🤖 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.
Inline comments:
In `@internal/controller/modules/modules_controller_actions.go`:
- Around line 567-568: The stale-generation path that handles observedGeneration
< generation returns before updating submodule status. Update the relevant
controller flow around mirrorSubmoduleConditions so that branch also writes
Status.Components.{ModelsAsAService,BatchGateway}.ManagementState and DSC
submodule conditions, preferably through a shared helper, while preserving the
existing not-ready behavior and ensuring every return path mirrors submodule
status consistently.
---
Nitpick comments:
In `@internal/controller/modules/base.go`:
- Around line 183-198: Extract the duplicated reflection and ManagementState
assignment into a shared setComponentManagementState helper. In
internal/controller/modules/base.go:183-198, update
BaseHandler.WriteDSCComponentStatus to call it with b.Config.GVK.Kind; in
internal/controller/modules/modules_controller_actions.go:692-711, update
writeSubmoduleComponentStatus to call it with sm.StatusFieldName while
preserving the existing platformCtx.DSC nil and empty-name guards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 38e57d32-3b15-412e-8d48-e2c106a345ba
📒 Files selected for processing (10)
api/components/v1alpha1/aigateway_types.goapi/components/v1alpha1/zz_generated.deepcopy.goapi/datasciencecluster/v2/datasciencecluster_types.goapi/datasciencecluster/v2/zz_generated.deepcopy.gointernal/controller/modules/aigateway/handler.gointernal/controller/modules/base.gointernal/controller/modules/export_test.gointernal/controller/modules/modules_controller_actions.gointernal/controller/modules/submodule_conditions_test.gointernal/controller/modules/types.go
|
checked and verified the status of the submodule comes through with one submodule enabled and another disabled. |
carlkyrillos
changed the title
| @@ -0,0 +1,26 @@ | |||
| package modules | |||
There was a problem hiding this comment.
Can we avoid this test file, and use package modules in internal/controller/modules/submodule_conditions_test.go?
…he same way on all modules
| WithCustomErrorMsg("ai-gateway-operator Deployment should have APPLICATIONS_NAMESPACE=%s injected", tc.AppsNamespace), | ||
| ) | ||
| }}, | ||
| {"Validate releases mirrored to DSC", func(t *testing.T) { |
There was a problem hiding this comment.
should we also add a similar test to mcplifecycleoperator?
|
@davidebianchi , looks like there are conflicts? |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carlkyrillos, cgoodfred 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3838 +/- ##
=======================================
Coverage ? 58.83%
=======================================
Files ? 224
Lines ? 17703
Branches ? 0
=======================================
Hits ? 10415
Misses ? 6332
Partials ? 956 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d4fbaf8
into
opendatahub-io:main
|
/early-gate-build |
Project workbenchNamespace from the Workbenches module CR onto status.components.workbenches via an optional workbenches-only writer, so the dashboard can resolve the legacy notebooks namespace without changing the generic release/managementState mirroring from opendatahub-io#3838. Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Project workbenchNamespace onto status.components.workbenches via an optional workbenches-only writer, so the dashboard can resolve the legacy notebooks namespace without changing the generic release/managementState mirroring from opendatahub-io#3838. Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Project workbenchNamespace onto status.components.workbenches via an optional workbenches-only writer, so the dashboard can resolve the legacy notebooks namespace without changing the generic release/managementState mirroring from opendatahub-io#3838. Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Project workbenchNamespace onto status.components.workbenches via an optional workbenches-only writer, so the dashboard can resolve the legacy notebooks namespace without changing the generic release/managementState mirroring from opendatahub-io#3838. Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Project workbenchNamespace onto status.components.workbenches via an optional workbenches-only writer, so the dashboard can resolve the legacy notebooks namespace without changing the generic release/managementState mirroring from #3838. Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
7 participants
Description
Module submodule status mirroring for DSC
Tested with AGO latest image to include the status reporting PR from their side.
How Has This Been Tested?
ModelsAsServiceReadyandBatchGatewayReadyScreenshot or short clip
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification
Covered with unit tests.
Summary by CodeRabbit
New Features
Documentation
Tests