Repository navigation
feat(workbenches): mirror workbenchNamespace into DSC status - #3890
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds an optional module hook for writing legacy DataScienceCluster status fields. Module reconciliation invokes the hook for enabled and disabled modules. The Workbenches handler mirrors the spec namespace into status, clears it when disabled or empty, and initializes component status as needed. Unit and end-to-end tests cover namespace mirroring, clearing, error handling, and application namespace placement. Estimated code review effort: 3 (Moderate) | ~20 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/workbenches/handler_test.go (1)
189-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover clearing when the namespace becomes empty.
The implementation clears on
!enabled || workbenchNamespace == "", but this test only exercisesenabled=falsewith a non-empty argument. Add anenabled=true→ empty-value case.Suggested test
+func TestWriteDSCWorkbenchNamespace_ClearsWhenEmpty(t *testing.T) { + g := NewWithT(t) + h := NewHandler() + dsc := &dscv2.DataScienceCluster{} + + h.WriteDSCWorkbenchNamespace(dsc, true, "rhods-notebooks") + h.WriteDSCWorkbenchNamespace(dsc, true, "") + + g.Expect(dsc.Status.Components.Workbenches.WorkbenchNamespace).Should(BeEmpty()) +}🤖 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/workbenches/handler_test.go` around lines 189 - 199, Extend TestWriteDSCWorkbenchNamespace_ClearsWhenDisabled to also call WriteDSCWorkbenchNamespace with enabled=true and an empty namespace after setting a non-empty namespace. Assert WorkbenchNamespace is empty, covering clearing when the namespace value becomes empty independently of the disabled case.
🤖 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/base.go`:
- Line 330: Update the status.workbenchNamespace extraction in the surrounding
controller flow to capture and propagate the error returned by
unstructured.NestedString instead of discarding it; only call
writeDSCWorkbenchNamespace when parsing succeeds. Add a malformed-status test
case verifying the accessor error is returned and the existing workbench
namespace is not cleared.
---
Nitpick comments:
In `@internal/controller/modules/workbenches/handler_test.go`:
- Around line 189-199: Extend TestWriteDSCWorkbenchNamespace_ClearsWhenDisabled
to also call WriteDSCWorkbenchNamespace with enabled=true and an empty namespace
after setting a non-empty namespace. Assert WorkbenchNamespace is empty,
covering clearing when the namespace value becomes empty independently of the
disabled case.
🪄 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: Pro Plus
Run ID: a9397f91-efbf-46ec-a00d-18ab7753f326
📒 Files selected for processing (6)
internal/controller/modules/base.gointernal/controller/modules/modules_controller_actions.gointernal/controller/modules/types.gointernal/controller/modules/workbenches/handler.gointernal/controller/modules/workbenches/handler_test.gotests/e2e/workbenches_test.go
fb76f21 to
4cfd189
Compare
4cfd189 to
ecba7e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 604-606: Update
internal/controller/modules/workbenches/handler.go:146-168 so
WriteLegacyStatusFields uses WorkbenchesCommonStatus.WorkbenchNamespace from the
Workbenches CR status rather than
dsc.Spec.Components.Workbenches.WorkbenchNamespace, passing that observed value
through the legacy writer contract as needed. Update
internal/controller/modules/modules_controller_actions.go:604-606 to mirror the
same observed WorkbenchNamespace value when writing legacy status fields;
preserve existing error handling.
🪄 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: Pro Plus
Run ID: c4914ea3-386e-4c0b-bf75-f65e11c707a0
📒 Files selected for processing (6)
internal/controller/modules/modules_controller_actions.gointernal/controller/modules/modules_controller_actions_test.gointernal/controller/modules/types.gointernal/controller/modules/workbenches/handler.gointernal/controller/modules/workbenches/handler_test.gotests/e2e/workbenches_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/modules/types.go
| // namespace. | ||
| // | ||
| // TODO: Remove once dashboard reads workbenchNamespace directly from the Workbenches | ||
| // CR (workbenches-operator status) instead of DSC status. |
There was a problem hiding this comment.
do we have a jira tracker for this?
jstourac
left a comment
jstourac
left a comment
There was a problem hiding this comment.
/lgtm
I've put one comment, otherwise LGTM. I'm not sure about the e2e test failures - doesn't seem to be related to the changes here on the first sight, but I didn't go deep.
|
The e2e tests seems to be failing due to this error: Seems like someone worked on fix: #3847 same as: #3882 (comment) |
|
@harshad16 the ray sha has since been updated here, but not sure if that contains the fix, checking |
|
The issue is related to a sha upgrade, but we need before ray to sync downstream to have e2e working again. It seems also a feastoperator permission error |
@asmigala , seems like some more fix went in yesterday opendatahub-io/kuberay#224 |
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>
ecba7e7 to
de08b0f
Compare
|
/test opendatahub-operator-e2e Restarting tests now that openshift/release#82675 has merged |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3890 +/- ##
==========================================
+ Coverage 59.62% 59.71% +0.09%
==========================================
Files 217 217
Lines 17002 17032 +30
==========================================
+ Hits 10137 10171 +34
+ Misses 5950 5944 -6
- Partials 915 917 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/hold |
|
/unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carlkyrillos, cgoodfred, davidebianchi 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 |
573ae31
into
opendatahub-io:main
6 participants
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 #3838.
Description
Add a workbenches-only optional hook on top of the existing #3838 WriteDSCComponentStatus flow:
Note: This intentionally does not restore full typed status projection or change how other modules mirror status into the DSC.
JIRA: https://redhat.atlassian.net/browse/RHOAIENG-79812
Related-to: opendatahub-io/workbenches-operator#77
How Has This Been Tested?
oc get dsc default-dsc -o jsonpath='{.status.components.workbenches.workbenchNamespace}{"\n"}'Expected: rhods-notebooksScreenshot 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
Summary by CodeRabbit