chore: remove LegacyComponentName on components which does not need 2 names #2017
Conversation
…wo names - we init. added LegacyCmponentName when we rename componentName - for backwards compatibility, we use Legacy name so the immutiable part in deployment will work - for these components, esp newly added one, there is no need to have two variable if componentName is enough Signed-off-by: Wen Zhou <[email protected]>
WalkthroughThis update removes all references to legacy component name constants across multiple controller components, replacing them with the current component name constants. The changes affect status updates, label selectors, and resource labeling in reconciliation logic. Associated legacy constants and comments are deleted from support files, with no changes to public APIs or exported function signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant Kustomize
participant Cluster
User->>Controller: Initiate reconciliation
Controller->>Kustomize: Render manifests with labels (ComponentName)
Kustomize->>Controller: Return rendered resources
Controller->>Cluster: Apply resources with labels (ComponentName)
Controller->>Cluster: Update status using ComponentName
Cluster-->>Controller: Status updated
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[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 |
![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 (21)
internal/controller/components/codeflare/codeflare.go(1 hunks)internal/controller/components/codeflare/codeflare_controller.go(1 hunks)internal/controller/components/codeflare/codeflare_support.go(0 hunks)internal/controller/components/kserve/kserve.go(1 hunks)internal/controller/components/kserve/kserve_controller.go(2 hunks)internal/controller/components/kserve/kserve_controller_actions.go(1 hunks)internal/controller/components/kueue/kueue.go(1 hunks)internal/controller/components/kueue/kueue_controller.go(1 hunks)internal/controller/components/kueue/kueue_support.go(0 hunks)internal/controller/components/ray/ray.go(1 hunks)internal/controller/components/ray/ray_controller.go(1 hunks)internal/controller/components/ray/ray_support.go(0 hunks)internal/controller/components/trainingoperator/trainingoperator.go(1 hunks)internal/controller/components/trainingoperator/trainingoperator_controller.go(1 hunks)internal/controller/components/trainingoperator/trainingoperator_support.go(0 hunks)internal/controller/components/trustyai/trustyai.go(1 hunks)internal/controller/components/trustyai/trustyai_controller.go(2 hunks)internal/controller/components/trustyai/trustyai_support.go(0 hunks)internal/controller/components/workbenches/workbenches.go(1 hunks)internal/controller/components/workbenches/workbenches_controller.go(2 hunks)internal/controller/components/workbenches/workbenches_support.go(0 hunks)
💤 Files with no reviewable changes (6)
- internal/controller/components/workbenches/workbenches_support.go
- internal/controller/components/trustyai/trustyai_support.go
- internal/controller/components/trainingoperator/trainingoperator_support.go
- internal/controller/components/kueue/kueue_support.go
- internal/controller/components/ray/ray_support.go
- internal/controller/components/codeflare/codeflare_support.go
🧰 Additional context used
🧬 Code Graph Analysis (9)
internal/controller/components/trainingoperator/trainingoperator_controller.go (3)
pkg/controller/predicates/component/component.go (1)
ForLabel(10-22)pkg/metadata/labels/types.go (3)
ODH(26-34)True(12-12)K8SCommon(19-23)internal/controller/components/trainingoperator/trainingoperator_support.go (1)
ComponentName(11-11)
internal/controller/components/trustyai/trustyai.go (2)
api/common/types.go (1)
Status(132-140)internal/controller/components/trustyai/trustyai_support.go (2)
ComponentName(13-13)ReadyConditionType(15-15)
internal/controller/components/trainingoperator/trainingoperator.go (1)
internal/controller/components/trainingoperator/trainingoperator_support.go (2)
ComponentName(11-11)ReadyConditionType(13-13)
internal/controller/components/kueue/kueue.go (1)
internal/controller/components/kueue/kueue_support.go (2)
ComponentName(11-11)ReadyConditionType(13-13)
internal/controller/components/kserve/kserve_controller.go (3)
pkg/controller/predicates/component/component.go (1)
ForLabel(10-22)pkg/metadata/labels/types.go (3)
ODH(26-34)True(12-12)K8SCommon(19-23)internal/controller/services/certconfigmapgenerator/certconfigmapgenerator_support.go (1)
PartOf(29-29)
internal/controller/components/workbenches/workbenches.go (2)
internal/controller/components/workbenches/workbenches_support.go (2)
ComponentName(13-13)ReadyConditionType(15-15)api/components/v1alpha1/workbenches_types.go (1)
WorkbenchesCommonStatus(56-59)
internal/controller/components/trustyai/trustyai_controller.go (3)
pkg/controller/predicates/component/component.go (1)
ForLabel(10-22)pkg/metadata/labels/types.go (3)
ODH(26-34)True(12-12)K8SCommon(19-23)internal/controller/components/trustyai/trustyai_support.go (1)
ComponentName(13-13)
internal/controller/components/kueue/kueue_controller.go (3)
pkg/controller/predicates/component/component.go (1)
ForLabel(10-22)pkg/metadata/labels/types.go (3)
ODH(26-34)True(12-12)K8SCommon(19-23)internal/controller/components/kueue/kueue_support.go (1)
ComponentName(11-11)
internal/controller/components/codeflare/codeflare.go (1)
internal/controller/components/codeflare/codeflare_support.go (2)
ComponentName(13-13)ReadyConditionType(15-15)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests and collect coverage
- GitHub Check: golangci-lint
🔇 Additional comments (19)
internal/controller/components/trainingoperator/trainingoperator.go (2)
81-81: Replace legacy component name with currentComponentNamefor status reset
The initial reset now correctly uses theComponentNameconstant instead of the removed legacy name.
89-89: Replace legacy component name with currentComponentNamewhen marked managed
Marking the component installed state now usesComponentName, ensuring consistency with the other status updates.internal/controller/components/trainingoperator/trainingoperator_controller.go (2)
56-56: Update predicate to use currentComponentNamelabel
The watch predicate now filters onlabels.ODH.Component(ComponentName)instead of the legacy constant, aligning with the simplified naming convention.
63-64: Apply currentComponentNamein kustomize labels
Rendering action now labels resources withODH.Component(ComponentName)andK8SCommon.PartOfset toComponentName, replacing the previous legacy labels.internal/controller/components/codeflare/codeflare.go (1)
82-82: LGTM! Consistent replacement of legacy component name.The changes correctly replace
LegacyComponentNamewithComponentNamein the status updates. The logic remains sound - initially marking the component as not installed, then conditionally marking it as installed based on management state.Also applies to: 90-90
internal/controller/components/codeflare/codeflare_controller.go (1)
64-64: LGTM! Controller setup consistently updated.The changes correctly update the controller reconciliation setup to use
ComponentNameinstead ofLegacyComponentNamein:
- CRD watch predicates (line 64)
- Kustomize action labels (lines 71-72)
This maintains consistency in component identification throughout the reconciliation logic.
Also applies to: 71-72
internal/controller/components/kueue/kueue_controller.go (2)
82-82: LGTM: Correctly updated component label selector.The change from
LegacyComponentNametoComponentNamein the CustomResourceDefinition watch predicate is correct and aligns with the PR objective of removing unnecessary dual naming conventions.
90-91: LGTM: Consistently updated kustomize action labels.Both label assignments correctly use
ComponentNameinstead ofLegacyComponentNamefor:
- ODH component labeling (
labels.ODH.Component(ComponentName))- Kubernetes part-of labeling (
labels.K8SCommon.PartOf, ComponentName)This ensures consistent resource labeling across the kueue component.
internal/controller/components/kueue/kueue.go (2)
82-82: LGTM: Correctly updated status tracking.The change from
LegacyComponentNametoComponentNamein the status initialization is correct and maintains the same functionality while using the current component naming convention.
90-90: LGTM: Status update properly aligned with component naming.The change to use
ComponentNamefor setting the installed component status when management state isManagedis correct and consistent with the initialization on line 82.internal/controller/components/trustyai/trustyai.go (1)
84-84: LGTM: Consistent replacement of legacy component name in status updates.The changes correctly replace
LegacyComponentNamewithComponentNamein theInstalledComponentsstatus map, ensuring consistent use of the current component name throughout the status management logic.Also applies to: 92-92
internal/controller/components/trustyai/trustyai_controller.go (1)
59-59: LGTM: Consistent component naming in reconciler setup.The changes correctly update the reconciler configuration to use
ComponentNameinstead ofLegacyComponentNamein:
- Label predicates for CRD event filtering (line 59)
- Kustomize action labels for both ODH component and K8S part-of labels (lines 74-75)
This ensures consistent component naming throughout the controller's event handling and resource management.
Also applies to: 74-75
internal/controller/components/workbenches/workbenches.go (1)
92-92: LGTM! Status reporting key updated from legacy to current component name.The changes correctly replace
LegacyComponentNamewithComponentNamein the status map operations, aligning with the PR objective to simplify naming conventions for components that don't require dual naming.Also applies to: 100-100
internal/controller/components/workbenches/workbenches_controller.go (1)
62-62:❓ Verification inconclusive
LGTM! Controller resource selection updated to use current component name.
The changes correctly replace
LegacyComponentNamewithComponentNamein:
- Label predicate for watching CustomResourceDefinitions (line 62)
- Kustomize action labels for resource management (lines 73-74)
This ensures the controller now targets and manages resources labeled with the current component name, consistent with the PR objective.
Verify that existing workbench resources in deployed environments use the current component name labels, or ensure proper migration if needed:
Also applies to: 73-74
🏁 Script executed:
#!/bin/bash # Description: Check for resources labeled with workbenches component names # Expected: Resources should be labeled with current component name, not legacy # Search for workbenches-related label usage in manifests rg -A 2 -B 2 "app\.opendatahub\.io/component.*workbenches" # Look for any legacy component name usage that might have been missed rg -i "legacy.*workbenches|workbenches.*legacy"Length of output: 110
Verify workbenches component labels in deployed environments
The update correctly replaces
LegacyComponentNamewithComponentNamein:
- Label predicate for watching CRDs (line 62)
- Kustomize action labels for resource management (lines 73–74)
However, our search did not find any occurrences of either the current or legacy workbenches labels in the repository manifests. To avoid disruption, please manually verify that existing workbench resources in your clusters are labeled correctly or have been migrated. For example:
# Check for current workbenches label on CRDs kubectl get customresourcedefinitions \ -l app.opendatahub.io/component=workbenches \ --show-labels # Check for any remaining legacy workbenches labels kubectl get customresourcedefinitions \ -l app.opendatahub.io/component=legacy-workbenches \ --show-labelsinternal/controller/components/kserve/kserve_controller_actions.go (1)
119-119:✅ Verification successful
Verify that no dev flag manifests use the legacy component name.
The filtering condition now only checks for
componentNamein manifest URIs, removing the previous check forLegacyComponentName. Ensure that all existing dev flag manifests have been updated to use the current component name in their URIs.
🏁 Script executed:
#!/bin/bash # Description: Search for any manifest files or configurations that might still reference the legacy Kserve component name # Expected: No references to legacy component names in manifest URIs or dev flag configurations # Search for potential legacy component name patterns in manifest files rg -i "kserve.*legacy|legacy.*kserve" --type yaml --type json # Search for old component naming patterns in dev flag related files rg -A 5 -B 5 "LegacyComponentName|legacy.*component" --type go # Look for any manifest URI configurations that might use old naming ast-grep --pattern 'URI: $_' | rg -i kserveLength of output: 31139
🏁 Script executed:
#!/bin/bash # Locate the defined ComponentName and LegacyComponentName for kserve rg -Hn "ComponentName" -Hn "LegacyComponentName" internal/controller/components/kserve/*.goLength of output: 821
🏁 Script executed:
#!/bin/bash # Search for ComponentName and LegacyComponentName definitions in kserve component files rg -Hn -e 'ComponentName\s*=' -e 'LegacyComponentName\s*=' internal/controller/components/kserveLength of output: 98
No legacy component name in KServe component
The KServe component does not define a
LegacyComponentNameconstant, so the dual‐check was never applicable. Removing the legacy check has no impact on existing dev-flag manifests—no manifests need updating.internal/controller/components/kserve/kserve_controller.go (1)
95-95: LGTM: Consistent use of current component name in reconciler setup.The changes correctly replace hardcoded legacy component name references with the dynamic
componentNamevariable in:
- CRD watching predicate (line 95)
- Kustomize action labels (lines 151-152)
This ensures consistent labeling and filtering based on the current component name throughout the reconciler lifecycle.
Also applies to: 151-152
internal/controller/components/kserve/kserve.go (1)
112-112: LGTM: Status updates use consistent component naming.The changes correctly update the DataScienceCluster status tracking to use
componentNameinstead ofLegacyComponentName. This maintains the same logic for marking component installation status while using consistent naming conventions.Also applies to: 120-120
internal/controller/components/ray/ray_controller.go (2)
66-67: Approve the kustomize label updates.The consistent replacement of
LegacyComponentNamewithComponentNamein the kustomize actions ensures uniform labeling across resources.
59-59:✅ Verification successful
Verify backwards compatibility for the predicate change.
The change from
LegacyComponentNametoComponentNamein the predicate looks correct, but ensure that existing CustomResourceDefinitions with the legacy component label will still be properly watched.
🏁 Script executed:
#!/bin/bash # Description: Verify ComponentName is defined and check for any remaining LegacyComponentName references # Expected: ComponentName constant exists, no LegacyComponentName references remain # Check if ComponentName is defined in ray component files echo "=== Checking ComponentName definition ===" ast-grep --pattern 'const ComponentName = $_' # Check for any remaining LegacyComponentName references echo "=== Checking for remaining LegacyComponentName references ===" rg -n "LegacyComponentName" --type go # Check existing CRD labels that might use the legacy name echo "=== Checking for existing Ray CRDs with legacy labels ===" rg -A 3 -B 3 "ray.*component.*label" --type yamlLength of output: 5617
No backward‐compatibility concerns for Ray predicate change
I’ve checked
internal/controller/components/rayand found no usages ofLegacyComponentName, so replacing it withComponentNamein the predicate won’t miss any existing Ray resources. No further action needed.
| @@ -79,15 +79,15 @@ func (s *componentHandler) UpdateDSCStatus(ctx context.Context, rr *types.Reconc | |||
| return cs, errors.New("failed to convert to DataScienceCluster") | |||
| } | |||
|
|
|||
| dsc.Status.InstalledComponents[LegacyComponentName] = false | |||
| dsc.Status.InstalledComponents[ComponentName] = false | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify backwards compatibility for status tracking changes.
The replacement of LegacyComponentName with ComponentName in the InstalledComponents status map ensures consistent component tracking. However, verify that existing DataScienceCluster resources won't be negatively impacted by this change.
Also applies to: 90-90
🏁 Script executed:
#!/bin/bash
# Description: Check existing DataScienceCluster resources for legacy component status entries
# Expected: Understand current usage patterns and potential impact
# Check for existing DataScienceCluster CRDs and instances
echo "=== Checking DataScienceCluster CRD definition ==="
ast-grep --pattern 'type DataScienceCluster struct {
$$$
}'
# Look for InstalledComponents field definition
echo "=== Checking InstalledComponents field usage ==="
rg -A 5 -B 5 "InstalledComponents" --type go
# Check for any other components that might have similar changes
echo "=== Checking other component status updates ==="
rg -A 2 -B 2 "Status\.InstalledComponents\[.*\]" --type goLength of output: 41710
Verify backward-compatibility of InstalledComponents key rename
We need to confirm whether migrating from LegacyComponentName to ComponentName will orphan or drop any legacy-named entries, and whether old keys linger or get cleaned up.
🏁 Script executed:
#!/bin/bash
# Locate computeComponentsStatus implementation
echo "=== Finding computeComponentsStatus ==="
rg -n "computeComponentsStatus" --type go
# Show its body to see how InstalledComponents is populated/cleared
echo "=== Dumping computeComponentsStatus implementation ==="
# Adjust path based on the previous rg output
sed -n '1,200p' internal/controller/datasciencecluster/datasciencecluster_controller_actions.go | sed -n '/func computeComponentsStatus/,/}/p'Length of output: 842
🏁 Script executed:
#!/bin/bash
# Dump the computeComponentsStatus implementation to see how InstalledComponents is managed
echo "=== computeComponentsStatus implementation ==="
sed -n '/func computeComponentsStatus/,/^}$/p' internal/controller/datasciencecluster/datasciencecluster_controller_support.goLength of output: 1723
Ensure legacy status keys don’t linger after rename
The switch from LegacyComponentName to ComponentName in each component’s UpdateDSCStatus will correctly set the new key, but it does not remove the old key from Status.InstalledComponents. On upgrade of an existing DataScienceCluster, both the legacy and new entries will coexist—potentially confusing users or automation that reads this map.
Please add a migration/cleanup step (e.g., in computeComponentsStatus or at the start of each UpdateDSCStatus) to remove any legacy-named keys once the new key is set:
• File: internal/controller/components/ray/ray.go
Lines: ~82–90
Example cleanup in computeComponentsStatus (in internal/controller/datasciencecluster/datasciencecluster_controller_support.go):
// before iterating components
for legacy := range instance.Status.InstalledComponents {
if legacy == LegacyComponentName || legacy == LegacyComponentNameUpstream {
delete(instance.Status.InstalledComponents, legacy)
}
}That way, old keys are purged and only the current names remain.
🤖 Prompt for AI Agents
In internal/controller/components/ray/ray.go around lines 82 to 90, after
setting the new InstalledComponents key with ComponentName, add code to delete
any legacy keys such as LegacyComponentName or LegacyComponentNameUpstream from
the Status.InstalledComponents map. This cleanup ensures that old keys do not
linger alongside the new keys, preventing confusion. Implement this either at
the start of the UpdateDSCStatus function or in computeComponentsStatus to
remove legacy keys after setting the new key.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2017 +/- ##
=======================================
Coverage 36.04% 36.04%
=======================================
Files 85 85
Lines 7708 7708
=======================================
Hits 2778 2778
Misses 4672 4672
Partials 258 258 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR needs rebase. 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. |
|
@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. |
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria
Summary by CodeRabbit