update: cleanup reference to old dashboard AProfile and HWProfile #2698
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
WalkthroughThis PR removes hardware profile migration and reconciliation functionality from the dashboard and initialization controllers. Changes include eliminating dynamic watches for hardware profile CRDs, removing related type definitions, reconciliation logic, test coverage, and narrowing RBAC permissions to read-only access for affected resources. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
zdtsw
requested review from
carlkyrillos and
Copilot
and removed request for
ugiordan
There was a problem hiding this comment.
Pull Request Overview
This PR removes the deprecated migration logic for Dashboard's HardwareProfile and AcceleratorProfile custom resources. The migration code that automatically converted Dashboard HardwareProfiles to infrastructure HardwareProfiles is no longer needed. The changes reduce RBAC permissions for these Dashboard resources to read-only operations (get, list, watch), removes the associated CRD watcher and migration functions, and eliminates the corresponding test files.
Key Changes
- Removed automatic migration logic for Dashboard HardwareProfile resources
- Reduced RBAC permissions for acceleratorprofiles and hardwareprofiles to read-only
- Removed CRD watch for Dashboard HardwareProfile and AcceleratorProfile resources
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controller/dscinitialization/dscinitialization_controller.go | Removed CRD watcher for Dashboard AcceleratorProfile and HardwareProfile, and removed the associated watch handler function |
| internal/controller/datasciencecluster/kubebuilder_rbac.go | Updated RBAC annotations to change permissions from create/patch/delete/update to read-only (get/list/watch) for acceleratorprofiles and hardwareprofiles |
| internal/controller/components/dashboard/dashboard_controller_actions_test.go | Deleted test file containing migration logic tests for HardwareProfile conversion |
| internal/controller/components/dashboard/dashboard_controller_actions.go | Removed migration functions and type definitions for Dashboard HardwareProfile migration |
| internal/controller/components/dashboard/dashboard_controller.go | Removed Dashboard HardwareProfile watcher, AcceleratorProfile ownership, and migration action from the controller |
| config/rbac/role.yaml | Updated RBAC role to consolidate acceleratorprofiles and hardwareprofiles under read-only permissions |
| bundle/manifests/opendatahub-operator.clusterserviceversion.yaml | Updated CSV manifest with new RBAC permissions and refreshed createdAt timestamp |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/hold |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2698 +/- ##
==========================================
- Coverage 49.65% 49.64% -0.02%
==========================================
Files 144 144
Lines 10562 10483 -79
==========================================
- Hits 5245 5204 -41
+ Misses 4758 4726 -32
+ Partials 559 553 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zdtsw
force-pushed
the
cleanup_dashboard_old
branch
from
2da147c to
5dce19c
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundle/manifests/opendatahub-operator.clusterserviceversion.yaml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (3)
bundle/manifests/opendatahub-operator.clusterserviceversion.yaml (3)
620-625: RBAC correctly restricts HardwareProfile permissions to read-only.The permissions for
hardwareprofiles(alongsideacceleratorprofiles) are limited toget,list, andwatch, aligning with the PR objective to restrict old dashboard CR permissions during cleanup.
628-638: RBAC reassigns full CRUD permissions to odhapplications and odhdocuments.The resources have been moved to a dedicated RBAC rule with full permissions (
create,delete,get,list,patch,update,watch), supporting the cleanup objective.
1636-1650: Webhook definitions for HardwareProfile retained appropriately.Webhook configurations for dashboard-hardwareprofile-validator and hardware-profile mutating injectors remain in place, preserving webhook functionality as stated in the PR objectives.
Also applies to: 1781-1835
| capabilities: Full Lifecycle | ||
| categories: AI/Machine Learning, Big Data | ||
| certified: "False" | ||
| containerImage: quay.io/opendatahub/opendatahub-operator:v3.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
Container image version doesn't match operator version.
Line 85 references container image v3.0.0 while the operator version (line 1536) is bumped to 3.1.0. Update the container image to match the version bump.
- containerImage: quay.io/opendatahub/opendatahub-operator:v3.0.0
+ containerImage: quay.io/opendatahub/opendatahub-operator:v3.1.0Also applies to: 1536-1536
🏁 Script executed:
#!/bin/bash
# Extract container image version around line 85
echo "=== Container Image (lines 80-90) ==="
sed -n '80,90p' bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
echo ""
echo "=== Operator Version (lines 1530-1540) ==="
sed -n '1530,1540p' bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
echo ""
echo "=== All version references in file ==="
grep -n "version\|Version\|containerImage" bundle/manifests/opendatahub-operator.clusterserviceversion.yaml | head -20Length of output: 1646
Container image version doesn't match operator version.
Line 85 references container image v3.0.0 while the operator version (line 1535) is bumped to 3.1.0. Update the container image to match the version bump.
- containerImage: quay.io/opendatahub/opendatahub-operator:v3.0.0
+ containerImage: quay.io/opendatahub/opendatahub-operator:v3.1.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| containerImage: quay.io/opendatahub/opendatahub-operator:v3.0.0 | |
| containerImage: quay.io/opendatahub/opendatahub-operator:v3.1.0 |
🤖 Prompt for AI Agents
In bundle/manifests/opendatahub-operator.clusterserviceversion.yaml around line
85, the containerImage is pinned to
quay.io/opendatahub/opendatahub-operator:v3.0.0 while the operator version was
bumped to 3.1.0 at line 1535; update the containerImage tag to v3.1.0 so the
image version matches the operator CSV version.
ea9086b to
219d761
Compare
- we should keep the webhook part remain for a while - the upgrade part will be remain for a while too Signed-off-by: Wen Zhou <[email protected]>
|
@zdtsw: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
i am closing this PR based on:
|
3 participants
DO NOT MERGE TILL WE KNOW IF WE SHOULD SUPPORT UPGRADE FROM 2.25 TO 3.3 OR MUST GO WITH 3.0 FIRST
Description
How Has This Been Tested?
Screenshot 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
for code cleanup
Summary by CodeRabbit
Refactor
Security