🌱 upgrade add-on APIs to v1beta1 #353
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elgnay 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 |
WalkthroughMigrates addon API usage across the repository from open-cluster-management.io/api/addon/v1alpha1 to v1beta1, updating imports, client constructors (NewForConfig), informers (V1alpha1 → V1beta1), type references, function signatures, constants, and tests; adapts logic where Spec.InstallNamespace was removed (agentInstallNamespace/default behavior). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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. Comment |
|
/hold |
elgnay
force-pushed
the
support-add-v1beta1-api
branch
from
82f0e71 to
74ae87e
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
test/integration/cloudevents/agent_deploy_test.go (2)
219-227: Critical:InstallNamespacefield causes pipeline failure.The pipeline failure confirms this:
unknown field InstallNamespace in struct literal of type "open-cluster-management.io/api/addon/v1beta1".ManagedClusterAddOnSpec.In v1beta1,
ManagedClusterAddOnSpecno longer contains theInstallNamespacefield. The install namespace should be handled differently in v1beta1 (typically via status or callback mechanisms).🔎 Proposed fix
Remove the
InstallNamespacefield from the spec. If install namespace configuration is required, check the v1beta1 API documentation for the correct approach:addon := &addonapiv1beta1.ManagedClusterAddOn{ ObjectMeta: metav1.ObjectMeta{ Name: testAddonImpl.name, }, - Spec: addonapiv1beta1.ManagedClusterAddOnSpec{ - InstallNamespace: "default", - }, + Spec: addonapiv1beta1.ManagedClusterAddOnSpec{}, }
338-346: Critical: SameInstallNamespacefield issue.This test case also uses the non-existent
InstallNamespacefield and needs the same fix.test/integration/kube/agent_deploy_test.go (1)
208-216: Critical:InstallNamespacefield doesn't exist in v1beta1ManagedClusterAddOnSpec.This file has multiple occurrences of this issue (lines 208-215, 302-309, 461-468, 673-680, 834-841, 1028-1035, 1093-1100). All instances need to be updated to remove the
InstallNamespacefield from the spec.pkg/utils/addon_config.go (1)
16-33: Interface returns v1alpha1 while test implementation returns v1beta1.The
AddOnDeploymentConfigGetterinterface anddefaultAddOnDeploymentConfigGetter.Get()correctly return*addonapiv1alpha1.AddOnDeploymentConfig(lines 18, 31-32), keepingAddOnDeploymentConfigon v1alpha1 while migratingManagedClusterAddOnto v1beta1. However, the test fileaddon_config_test.gohastestadcGetter.Get()returning*addonapiv1beta1.AddOnDeploymentConfig, which will fail to compile.pkg/addonmanager/controllers/agentdeploy/default_hook_sync_test.go (1)
44-53: Update OwnerReference APIVersion to v1beta1.The
OwnerReference.APIVersionis set to"addon.open-cluster-management.io/v1alpha1"but should bev1beta1to match the rest of the file's v1beta1 migration. Update this at lines 46, 168, and 235 in the OwnerReference definitions. The same issue exists indefault_sync_test.goat line 189.pkg/addonfactory/helm_agentaddon_test.go (1)
196-207: UpdateToAddOnResourceRequirementsValuesto acceptaddonapiv1beta1.AddOnDeploymentConfig.The function currently accepts
addonapiv1alpha1.AddOnDeploymentConfigbut the test is passingaddonapiv1beta1.AddOnDeploymentConfig, which will cause a compilation error. The function signature inpkg/addonfactory/addondeploymentconfig.gomust be migrated to the beta1 type to match the API migration.test/integration/kube/suite_test.go (1)
67-68: Addaddon/v1beta1CRD directory path to match API usage.The test code uses
addonapiv1beta1API throughout, but the CRD directory configuration only includesaddon/v1alpha1. Following the pattern established for cluster resources (which include bothv1andv1beta1paths), addaddon/v1beta1to the CRD paths:filepath.Join(".", "vendor", "open-cluster-management.io", "api", "addon", "v1beta1"),This should be added alongside the existing
v1alpha1path to ensure all required CRDs are loaded for the test environment.
🧹 Nitpick comments (5)
pkg/addonmanager/cloudevents/manager.go (1)
15-16: Misleading import alias:addonv1alpha1importsv1beta1package.The alias
addonv1alpha1now importsopen-cluster-management.io/api/addon/v1beta1, which is confusing and could lead to maintenance issues. Consider renaming the alias to match the actual API version being imported.🔎 Proposed fix
- addonv1alpha1 "open-cluster-management.io/api/addon/v1beta1" - addonv1beta1client "open-cluster-management.io/api/client/addon/clientset/versioned" + addonv1beta1 "open-cluster-management.io/api/addon/v1beta1" + addonv1beta1client "open-cluster-management.io/api/client/addon/clientset/versioned"Then update references from
addonv1alpha1.AddonLabelKeytoaddonv1beta1.AddonLabelKeythroughout the file.pkg/addonmanager/controllers/certificate/csrapprove.go (1)
22-24: Same misleading alias pattern as other files.The alias
addonv1alpha1importsv1beta1. For consistency and clarity, consider renaming toaddonv1beta1across the codebase. This would make the code self-documenting about which API version is actually in use.pkg/addonmanager/manager.go (1)
14-15: Misleading import alias persists across files.Same issue as noted in other files -
addonv1alpha1imports the v1beta1 package. This creates a maintenance burden and potential confusion for future contributors.pkg/addonfactory/test_helper.go (1)
19-31: Remove the unusedinstallNamespaceparameter fromNewFakeManagedClusterAddon.The
installNamespaceparameter is accepted but never used sinceInstallNamespacewas removed from the v1beta1ManagedClusterAddOnSpec. All three call sites in the test suite—template_agentaddon_test.go,helm_agentaddon_test.go, andhelper_test.go—pass values that are silently ignored. Remove the parameter and update the three call sites accordingly.pkg/addonmanager/controllers/addonconfig/controller.go (1)
192-206: Consider handling the case where both DesiredConfig and LastAppliedConfig are nil.The current logic extracts
namespaceandnamefromDesiredConfigorLastAppliedConfig. If both are nil,namewill be an empty string, and the subsequentlister.Get("")orlister.Namespace(namespace).Get("")call would likely fail or return unexpected results.🔎 Suggested defensive check
if configReference.DesiredConfig != nil { namespace = configReference.DesiredConfig.Namespace name = configReference.DesiredConfig.Name } else if configReference.LastAppliedConfig != nil { namespace = configReference.LastAppliedConfig.Namespace name = configReference.LastAppliedConfig.Name } + + if name == "" { + // Skip if no config reference name is available + continue + } if namespace == "" { config, err = lister.Get(name)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (79)
cmd/example/helloworld/main.gocmd/example/helloworld_helm/main.goexamples/helloworld/helloworld.goexamples/helloworld/helloworld_test.goexamples/helloworld_agent/agent.goexamples/helloworld_helm/helloworld_helm.goexamples/helloworld_helm/helloworld_helm_test.goexamples/rbac/rbac.gopkg/addonfactory/addondeploymentconfig.gopkg/addonfactory/addondeploymentconfig_test.gopkg/addonfactory/addonfactory.gopkg/addonfactory/helm_agentaddon.gopkg/addonfactory/helm_agentaddon_test.gopkg/addonfactory/helper.gopkg/addonfactory/template_agentaddon.gopkg/addonfactory/template_agentaddon_test.gopkg/addonfactory/test_helper.gopkg/addonmanager/addontesting/helpers.gopkg/addonmanager/base_manager.gopkg/addonmanager/cloudevents/manager.gopkg/addonmanager/constants/constants.gopkg/addonmanager/controllers/addonconfig/controller.gopkg/addonmanager/controllers/addonconfig/controller_test.gopkg/addonmanager/controllers/agentdeploy/controller.gopkg/addonmanager/controllers/agentdeploy/default_hook_sync.gopkg/addonmanager/controllers/agentdeploy/default_hook_sync_test.gopkg/addonmanager/controllers/agentdeploy/default_sync.gopkg/addonmanager/controllers/agentdeploy/default_sync_test.gopkg/addonmanager/controllers/agentdeploy/healthcheck_sync.gopkg/addonmanager/controllers/agentdeploy/healthcheck_sync_test.gopkg/addonmanager/controllers/agentdeploy/hosted_hook_sync.gopkg/addonmanager/controllers/agentdeploy/hosted_hook_sync_test.gopkg/addonmanager/controllers/agentdeploy/hosted_sync.gopkg/addonmanager/controllers/agentdeploy/hosted_sync_test.gopkg/addonmanager/controllers/agentdeploy/util_test.gopkg/addonmanager/controllers/agentdeploy/utils.gopkg/addonmanager/controllers/certificate/csrapprove.gopkg/addonmanager/controllers/certificate/csrapprove_test.gopkg/addonmanager/controllers/certificate/csrsign.gopkg/addonmanager/controllers/certificate/csrsign_test.gopkg/addonmanager/controllers/cmaconfig/controller.gopkg/addonmanager/controllers/cmaconfig/controller_test.gopkg/addonmanager/controllers/cmamanagedby/controller.gopkg/addonmanager/controllers/cmamanagedby/controller_test.gopkg/addonmanager/controllers/registration/controller.gopkg/addonmanager/controllers/registration/controller_test.gopkg/addonmanager/manager.gopkg/agent/inteface.gopkg/index/index.gopkg/utils/addon.gopkg/utils/addon_config.gopkg/utils/addon_config_test.gopkg/utils/addon_test.gopkg/utils/csr_helper_test.gopkg/utils/csr_helpers.gopkg/utils/helpers.gopkg/utils/helpers_test.gopkg/utils/permission.gopkg/utils/permission_test.gopkg/utils/probe_helper.gotest/e2e/helloworld_helm_test.gotest/e2e/helloworld_test.gotest/e2ehosted/helloworld_hosted_test.gotest/integration/cloudevents/agent_deploy_test.gotest/integration/cloudevents/agent_hook_deploy_test.gotest/integration/cloudevents/agent_hosting_deploy_test.gotest/integration/cloudevents/agent_hosting_hook_deploy_test.gotest/integration/cloudevents/suite_test.gotest/integration/kube/agent_deploy_test.gotest/integration/kube/agent_hook_deploy_test.gotest/integration/kube/agent_hosting_deploy_test.gotest/integration/kube/agent_hosting_hook_deploy_test.gotest/integration/kube/assertion_test.gotest/integration/kube/cluster_management_addon_test.gotest/integration/kube/csr_test.gotest/integration/kube/deleted_managed_cluster_create_addon_test.gotest/integration/kube/multiworks_test.gotest/integration/kube/registration_test.gotest/integration/kube/suite_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T09:30:55.917Z
Learnt from: zhujian7
Repo: open-cluster-management-io/addon-framework PR: 334
File: pkg/addonmanager/base_manager.go:107-111
Timestamp: 2025-09-15T09:30:55.917Z
Learning: BaseAddonManagerImpl instances are designed to operate in a single mode: either templateBasedAddOn=true (handles only template-based addons) or templateBasedAddOn=false (handles all addons). A single manager instance does not handle mixed addon types - this architectural separation ensures clean lifecycle management.
Applied to files:
pkg/addonmanager/base_manager.go
📚 Learning: 2025-09-15T08:14:41.882Z
Learnt from: zhujian7
Repo: open-cluster-management-io/addon-framework PR: 334
File: pkg/addonmanager/base_manager.go:83-87
Timestamp: 2025-09-15T08:14:41.882Z
Learning: In template-based addon mode (templateBasedAddOn=true), the FilterTemplateBasedAddOns function intentionally returns false for template-based addons whose Status.ConfigReferences are not yet populated with the proper AddOnTemplate reference and SpecHash. This ensures controllers retry and wait for template configuration to be fully established before processing, preventing premature processing of incompletely configured template addons.
Applied to files:
pkg/utils/addon_test.gotest/integration/cloudevents/agent_deploy_test.gopkg/utils/addon.gopkg/addonmanager/controllers/addonconfig/controller_test.gopkg/utils/addon_config.gotest/integration/kube/agent_hosting_hook_deploy_test.gopkg/addonmanager/controllers/agentdeploy/utils.gotest/integration/cloudevents/agent_hosting_hook_deploy_test.go
🧬 Code graph analysis (28)
examples/helloworld/helloworld.go (1)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)
pkg/addonmanager/base_manager.go (3)
pkg/utils/helpers.go (1)
FilterByAddonName(311-317)pkg/addonmanager/controllers/cmaconfig/controller.go (1)
NewCMAConfigController(47-79)pkg/addonmanager/controllers/certificate/csrsign.go (1)
NewCSRSignController(40-81)
pkg/utils/probe_helper.go (1)
pkg/agent/inteface.go (1)
FieldResult(205-211)
pkg/addonfactory/helper.go (1)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)
examples/rbac/rbac.go (1)
pkg/agent/inteface.go (1)
PermissionConfigFunc(110-110)
pkg/utils/helpers.go (1)
pkg/addonmanager/constants/constants.go (2)
AddonLifecycleAnnotationKey(20-20)AddonLifecycleAddonManagerAnnotationValue(24-24)
examples/helloworld_helm/helloworld_helm_test.go (1)
pkg/addonmanager/addontesting/helpers.go (1)
NewAddon(81-89)
pkg/addonfactory/addondeploymentconfig.go (1)
pkg/utils/addon_config.go (2)
NewAddOnDeploymentConfigGetter(26-28)AddOnDeploymentConfigGetter(17-19)
test/integration/cloudevents/agent_deploy_test.go (1)
pkg/utils/helpers.go (1)
MergeOwnerRefs(189-225)
examples/helloworld/helloworld_test.go (1)
pkg/addonmanager/addontesting/helpers.go (1)
NewAddon(81-89)
pkg/addonmanager/controllers/agentdeploy/default_hook_sync.go (1)
pkg/agent/inteface.go (1)
AgentAddon(19-36)
examples/helloworld_helm/helloworld_helm.go (1)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)
pkg/addonmanager/controllers/addonconfig/controller_test.go (1)
pkg/addonmanager/addontesting/helpers.go (1)
NewAddon(81-89)
pkg/addonmanager/controllers/registration/controller.go (2)
pkg/agent/inteface.go (1)
AgentAddon(19-36)pkg/utils/addon.go (1)
ManagedClusterAddOnFilterFunc(10-10)
pkg/addonfactory/template_agentaddon.go (1)
pkg/addonfactory/addonfactory.go (2)
Values(25-25)AddonDefaultInstallNamespace(20-20)
pkg/addonmanager/controllers/certificate/csrapprove_test.go (1)
pkg/agent/inteface.go (2)
AgentAddonOptions(39-99)RegistrationOption(119-162)
pkg/addonfactory/template_agentaddon_test.go (1)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)
test/integration/kube/agent_hosting_deploy_test.go (1)
pkg/addonmanager/constants/constants.go (1)
InstallModeHosted(14-14)
pkg/utils/addon_config.go (1)
pkg/addonfactory/addondeploymentconfig.go (1)
NewAddOnDeploymentConfigGetter(195-197)
pkg/addonmanager/controllers/cmamanagedby/controller_test.go (3)
pkg/addonmanager/constants/constants.go (2)
AddonLifecycleAnnotationKey(20-20)AddonLifecycleSelfManageAnnotationValue(28-28)pkg/addonmanager/addontesting/helpers.go (1)
AssertActions(297-306)pkg/utils/helpers.go (1)
FilterByAddonName(311-317)
test/integration/cloudevents/suite_test.go (1)
pkg/addonmanager/constants/constants.go (1)
GetHostedModeInfo(52-62)
pkg/addonmanager/controllers/agentdeploy/utils.go (1)
pkg/agent/inteface.go (1)
AgentAddon(19-36)
pkg/utils/addon_config_test.go (1)
pkg/utils/addon_config.go (1)
AddOnDeploymentConfigGetter(17-19)
test/integration/kube/assertion_test.go (2)
pkg/addonmanager/constants/constants.go (2)
AddonLifecycleAnnotationKey(20-20)AddonLifecycleAddonManagerAnnotationValue(24-24)pkg/utils/helpers.go (1)
MergeOwnerRefs(189-225)
pkg/addonmanager/controllers/agentdeploy/healthcheck_sync.go (1)
pkg/agent/inteface.go (3)
HealthProberTypeLease(223-223)HealthProber(172-176)HealthProberTypeWork(229-229)
pkg/addonmanager/controllers/certificate/csrsign_test.go (1)
pkg/agent/inteface.go (2)
AgentAddonOptions(39-99)RegistrationOption(119-162)
pkg/addonmanager/controllers/cmaconfig/controller_test.go (1)
pkg/addonmanager/addontesting/helpers.go (1)
NewClusterManagementAddon(139-154)
pkg/addonfactory/helm_agentaddon_test.go (2)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)pkg/addonfactory/addondeploymentconfig.go (1)
ToAddOnResourceRequirementsValues(141-169)
🪛 GitHub Actions: CloudEventsIntegration
test/integration/cloudevents/agent_deploy_test.go
[error] 224-224: unknown field InstallNamespace in struct literal of type "open-cluster-management.io/api/addon/v1beta1".ManagedClusterAddOnSpec. See failure during 'go test -c ./test/integration/cloudevents -o ./cloudevents-integration.test'.
🪛 GitHub Check: integration
test/integration/kube/assertion_test.go
[failure] 41-41:
undefined: addonapiv1beta1.CustomizedVariable
[failure] 40-40:
undefined: addonapiv1beta1.AddOnDeploymentConfigSpec
[failure] 33-33:
undefined: addonapiv1beta1.CustomizedVariable
[failure] 32-32:
undefined: addonapiv1beta1.AddOnDeploymentConfigSpec
[failure] 25-25:
undefined: addonapiv1beta1.CustomizedVariable
[failure] 24-24:
undefined: addonapiv1beta1.AddOnDeploymentConfigSpec
⏰ 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). (6)
- GitHub Check: e2e-cloudevents
- GitHub Check: unit
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: verify
| func (b *clusterManagementAddonBuilder) WithSupportedConfigs(supportedConfigs ...addonapiv1beta1.ConfigGroupResource) *clusterManagementAddonBuilder { | ||
| // In v1beta1, use DefaultConfigs instead of SupportedConfigs | ||
| // Convert ConfigGroupResource to AddOnConfig | ||
| for _, cfg := range supportedConfigs { | ||
| b.clusterManagementAddOn.Spec.DefaultConfigs = append(b.clusterManagementAddOn.Spec.DefaultConfigs, | ||
| addonapiv1beta1.AddOnConfig{ | ||
| ConfigGroupResource: cfg, | ||
| }) | ||
| } | ||
| return b | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find usages of WithSupportedConfigs to verify how it's being used in tests
rg -nP --type=go -C3 'WithSupportedConfigs\('Repository: open-cluster-management-io/addon-framework
Length of output: 707
🏁 Script executed:
# Also look for the API definitions to understand ConfigGroupResource and AddOnConfig structures
rg -nP --type=go 'type ConfigGroupResource struct|type AddOnConfig struct' -A 5Repository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Search for DefaultConfigs and SupportedConfigs usage to understand the API change
rg -nP --type=go 'DefaultConfigs|SupportedConfigs' -B 2 -A 2 | head -100Repository: open-cluster-management-io/addon-framework
Length of output: 2819
🏁 Script executed:
# Search for actual calls to WithSupportedConfigs in test files
rg -nP --type=go '\.WithSupportedConfigs\(' -B 2 -A 2Repository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Look at the API definitions more carefully - search for the struct definitions
rg -nP --type=go 'type ClusterManagementAddOnSpec|type ManagedClusterAddOnSpec' -A 15 | head -80Repository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Check if DefaultConfigs and SupportedConfigs are both in the same struct or different ones
rg -nP --type=go 'DefaultConfigs|SupportedConfigs' --type=go | grep -E '^\s*(DefaultConfigs|SupportedConfigs)\s*' | head -20Repository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Find the API definitions - look in common v1beta1 structure
find . -type f -name "*.go" | xargs rg -l "type.*ConfigGroupResource\|type.*AddOnConfig" | head -10Repository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Get more context from helpers.go to understand the builder
cat -n pkg/addonmanager/addontesting/helpers.go | sed -n '130,180p'Repository: open-cluster-management-io/addon-framework
Length of output: 2320
🏁 Script executed:
# Search for test files that might use the builder pattern
rg -l "NewClusterManagementAddon\|clusterManagementAddonBuilder" --type=goRepository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Look at the imports in helpers.go to understand the API
head -30 pkg/addonmanager/addontesting/helpers.goRepository: open-cluster-management-io/addon-framework
Length of output: 983
🏁 Script executed:
# Search for the API package definition
find . -path "*/api/*" -name "*.go" -type f | head -20Repository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Search for AddOnConfig and DefaultConfigs definitions
rg "type AddOnConfig|DefaultConfigs \[\]" --type=go -A 3 | head -50Repository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Search for how DefaultConfigs is used in the codebase
rg "DefaultConfigs" --type=go -B 2 -A 2 | head -60Repository: open-cluster-management-io/addon-framework
Length of output: 1342
🏁 Script executed:
# Search for ConfigReferent to understand what it does
rg "ConfigReferent" --type=go -B 2 -A 2Repository: open-cluster-management-io/addon-framework
Length of output: 28078
🏁 Script executed:
# Check if there are any recent changes or comments about the API migration
rg "v1beta1|v1alpha1|DefaultConfigs|SupportedConfigs" --type=go pkg/addonmanager/addontesting/helpers.go -C 1Repository: open-cluster-management-io/addon-framework
Length of output: 5270
🏁 Script executed:
# Search for AddOnConfig structure definition to understand required fields
rg "type AddOnConfig struct" -A 10 --type=goRepository: open-cluster-management-io/addon-framework
Length of output: 68
🏁 Script executed:
# Look at how DefaultConfigs is actually used in controllers
rg "\.DefaultConfigs\s*=" --type=go -B 2 -A 3Repository: open-cluster-management-io/addon-framework
Length of output: 588
The WithSupportedConfigs transformation is incomplete and non-functional.
The method creates AddOnConfig objects with only ConfigGroupResource populated but leaves ConfigReferent empty. Since ConfigReferent (containing Name and Namespace) is required to actually reference a configuration resource—as shown in all test usages across the codebase—the generated configs will not function correctly. This builder method is also currently unused in any tests. Either populate ConfigReferent properly or reconsider the approach for this helper.
🤖 Prompt for AI Agents
In pkg/addonmanager/addontesting/helpers.go around lines 156-166, the
WithSupportedConfigs builder currently creates AddOnConfig entries with only
ConfigGroupResource set leaving ConfigReferent empty; update the builder to
populate ConfigReferent so configs actually reference a resource—set
ConfigReferent.Name to a deterministic value (e.g., cfg.Resource or
cfg.Resource+"-config") and ConfigReferent.Namespace to the addon's namespace
(use b.clusterManagementAddOn.Namespace and fall back to "default" if empty),
ensuring each appended AddOnConfig includes both ConfigGroupResource and a valid
ConfigReferent (unique name if multiple entries) so tests can find the
referenced config objects.
| addon := &addonapiv1beta1.ManagedClusterAddOn{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: testAddonImpl.name, | ||
| }, | ||
| Spec: addonapiv1alpha1.ManagedClusterAddOnSpec{ | ||
| Spec: addonapiv1beta1.ManagedClusterAddOnSpec{ | ||
| InstallNamespace: "test", | ||
| }, | ||
| } | ||
| createManagedClusterAddOnwithOwnerRefs(managedClusterName, addon, cma) |
There was a problem hiding this comment.
Critical: InstallNamespace field doesn't exist in v1beta1.
Same issue as other test files - the InstallNamespace field in ManagedClusterAddOnSpec doesn't exist in v1beta1.
🤖 Prompt for AI Agents
In test/integration/kube/cluster_management_addon_test.go around lines 74-82,
the ManagedClusterAddOnSpec uses the non-existent InstallNamespace field for
v1beta1; remove the InstallNamespace assignment from Spec and instead set the
object's namespace via ObjectMeta.Namespace = "test" (or use the correct v1beta1
field if your API version exposes it), and ensure
createManagedClusterAddOnwithOwnerRefs call still receives the addon with the
namespace populated.
74ae87e to
db39f1b
Compare
Signed-off-by: Yang Le <[email protected]>
elgnay
force-pushed
the
support-add-v1beta1-api
branch
from
db39f1b to
1a75b98
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
pkg/addonmanager/controllers/certificate/csrsign.go (1)
29-37: Minor: stale comment on struct.Line 29 says "csrApprovingController auto approve..." but this struct is
csrSignController. The comment appears to be copy-pasted fromcsrapprove.go.🔎 Proposed fix
-// csrApprovingController auto approve the renewal CertificateSigningRequests for an accepted spoke cluster on the hub. +// csrSignController signs the approved CertificateSigningRequests for an accepted spoke cluster on the hub. type csrSignController struct {pkg/utils/addon_config_test.go (1)
99-133: Inconsistent API version usage in commented-out test case.The commented-out test case mixes API versions inconsistently:
- Line 102 references
addonapiv1beta1.AddOnDeploymentConfig- Line 106 uses
addonapiv1alpha1.AddOnDeploymentConfigSpecIf this test case is to be re-enabled in the future, ensure consistent usage of either alpha1 or beta1 throughout the entire test case structure.
🔎 Suggested fix for consistency
If keeping alpha1 for
AddOnDeploymentConfig(to match lines 13-25), update line 102:-// &addonapiv1beta1.AddOnDeploymentConfig{ +// &addonapiv1alpha1.AddOnDeploymentConfig{Or, if migrating to beta1, update lines 106 and the getter parameter:
-// Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{ +// Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{test/integration/cloudevents/suite_test.go (1)
119-127: Critical: v1beta1 CRDs are not installed in the test environment.The test environment includes CRDs for
addon/v1alpha1(line 125) but not foraddon/v1beta1. This causes the pipeline failure where the server cannot find v1beta1 resources. Since this PR migrates all addon API usage to v1beta1, the test environment must load the v1beta1 CRDs.🔎 Proposed fix to add v1beta1 CRD path
testEnv = &envtest.Environment{ ErrorIfCRDPathMissing: true, CRDDirectoryPaths: []string{ filepath.Join(".", "vendor", "open-cluster-management.io", "api", "work", "v1", "0000_00_work.open-cluster-management.io_manifestworks.crd.yaml"), filepath.Join(".", "vendor", "open-cluster-management.io", "api", "cluster", "v1"), filepath.Join(".", "vendor", "open-cluster-management.io", "api", "cluster", "v1beta1"), filepath.Join(".", "vendor", "open-cluster-management.io", "api", "addon", "v1alpha1"), + filepath.Join(".", "vendor", "open-cluster-management.io", "api", "addon", "v1beta1"), }, }pkg/addonmanager/controllers/agentdeploy/default_hook_sync_test.go (1)
44-53: Update OwnerReference APIVersion to v1beta1.The test hardcodes
APIVersion: "addon.open-cluster-management.io/v1alpha1"while production code usesaddonapiv1beta1.GroupVersion.String()(v1beta1). Update to"addon.open-cluster-management.io/v1beta1"to align with the actual API version used throughout the codebase and ensure the test properly reflects production ownership semantics. This affects lines 46, 168, and 235 in this file.test/integration/kube/suite_test.go (1)
63-69: Add v1beta1 CRD directory path for addon API.The code uses
addonv1beta1client(line 79) and importsaddonapiv1beta1types, but the CRD directory path references onlyaddon/v1alpha1(line 67). SinceManagedClusterAddOn,ClusterManagementAddOn, andRegistrationConfigare v1beta1 types being used throughout the tests, add a corresponding v1beta1 CRD directory path:filepath.Join(".", "vendor", "open-cluster-management.io", "api", "addon", "v1beta1"),Alternatively, verify that the
addon/v1alpha1directory already contains CRDs for both v1alpha1 and v1beta1 versions.pkg/agent/inteface.go (1)
241-257: Fix critical structural mismatch in RegistrationConfig usage.The code constructs
RegistrationConfigwithTypeandKubeClientfields, but these do not exist in the actual v1alpha1 API definition (the addon APIs are in v1alpha1, not v1beta1). The v1alpha1RegistrationConfigonly has two fields:
signerName(string)subject(Subject, optional)The
TypeandKubeClientfields actually belong toRegistrationSpec. The function should return[]addonapiv1beta1.RegistrationSpecinstead of[]addonapiv1beta1.RegistrationConfig, or restructure the return value to use the correct API contract.pkg/addonmanager/controllers/agentdeploy/default_sync_test.go (1)
187-196: Consider updating the owner reference API version to v1beta1.The owner reference in the test data still specifies
APIVersion: "addon.open-cluster-management.io/v1alpha1". Since this PR migrates to v1beta1, this should likely be updated to"addon.open-cluster-management.io/v1beta1"for consistency.🔎 Proposed fix
pTrue := true work.SetOwnerReferences([]metav1.OwnerReference{ { - APIVersion: "addon.open-cluster-management.io/v1alpha1", + APIVersion: "addon.open-cluster-management.io/v1beta1", Kind: "ManagedClusterAddOn", Name: "test", UID: "", Controller: &pTrue, BlockOwnerDeletion: &pTrue, }, })
♻️ Duplicate comments (4)
pkg/addonmanager/addontesting/helpers.go (1)
156-166: The incompleteWithSupportedConfigsimplementation persists.As noted in the previous review, this method creates
AddOnConfigobjects with onlyConfigGroupResourcepopulated while leavingConfigReferentempty. SinceConfigReferent(containing Name and Namespace) is required to actually reference a configuration resource, these configs remain non-functional. The previous review provided a detailed prompt for properly populatingConfigReferentwith deterministic values.test/integration/cloudevents/agent_hosting_hook_deploy_test.go (1)
132-142: Critical: AddOnDeploymentConfig should use v1beta1 API.Same issue as in agent_hook_deploy_test.go: this code creates an
AddOnDeploymentConfigusingaddonapiv1alpha1types and callshubAddonClient.AddonV1alpha1(), which is inconsistent with the v1beta1 migration.🔎 Proposed fix to use v1beta1 AddOnDeploymentConfig
- deployConfig := &addonapiv1alpha1.AddOnDeploymentConfig{ + deployConfig := &addonapiv1beta1.AddOnDeploymentConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-deploy-config", Namespace: managedClusterName, }, - Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{ + Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{ AgentInstallNamespace: "default", }, } - _, err = hubAddonClient.AddonV1alpha1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) + _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) gomega.Expect(err).ToNot(gomega.HaveOccurred())test/integration/cloudevents/agent_hosting_deploy_test.go (1)
127-137: Critical: AddOnDeploymentConfig should use v1beta1 API.This file has the same inconsistency:
AddOnDeploymentConfigis created using v1alpha1 types while the rest of the code uses v1beta1.🔎 Proposed fix to use v1beta1 AddOnDeploymentConfig
- deployConfig := &addonapiv1alpha1.AddOnDeploymentConfig{ + deployConfig := &addonapiv1beta1.AddOnDeploymentConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-deploy-config", Namespace: managedClusterName, }, - Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{ + Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{ AgentInstallNamespace: "default", }, } - _, err = hubAddonClient.AddonV1alpha1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) + _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) gomega.Expect(err).ToNot(gomega.HaveOccurred())test/integration/cloudevents/agent_deploy_test.go (1)
183-193: Critical: AddOnDeploymentConfig should use v1beta1 API.This file also has the same inconsistency with
AddOnDeploymentConfigusing v1alpha1 types and API calls.🔎 Proposed fix to use v1beta1 AddOnDeploymentConfig
- deployConfig := &addonapiv1alpha1.AddOnDeploymentConfig{ + deployConfig := &addonapiv1beta1.AddOnDeploymentConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-deploy-config", Namespace: managedClusterName, }, - Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{ + Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{ AgentInstallNamespace: "default", }, } - _, err = hubAddonClient.AddonV1alpha1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) + _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) gomega.Expect(err).ToNot(gomega.HaveOccurred())
🧹 Nitpick comments (6)
pkg/addonmanager/controllers/certificate/csrapprove.go (1)
22-24: Misleading import alias:addonv1alpha1points tov1beta1package.The alias
addonv1alpha1is used for theaddon/v1beta1package, which is confusing and could lead to maintenance issues. Consider renaming toaddonv1beta1for consistency with the other imports on lines 23-24.🔎 Proposed fix
- addonv1alpha1 "open-cluster-management.io/api/addon/v1beta1" + addonv1beta1 "open-cluster-management.io/api/addon/v1beta1"Note: This will require updating all references from
addonv1alpha1toaddonv1beta1throughout the file (lines 109, 137, 213).test/integration/kube/assertion_test.go (2)
96-107: Consider checking Get error before accessing result.The migration to v1beta1 is correct. However, if
Getfails,oldwill be nil and accessingold.Specwill panic. Whilegomega.Eventuallywill retry, explicit error handling would be cleaner.🔎 Proposed improvement
func updateClusterManagementAddOn(ctx context.Context, new *addonapiv1beta1.ClusterManagementAddOn) { gomega.Eventually(func() bool { old, err := hubAddonClient.AddonV1beta1().ClusterManagementAddOns().Get(context.Background(), new.Name, metav1.GetOptions{}) + if err != nil { + return false + } old.Spec = new.Spec old.Annotations = new.Annotations - _, err = hubAddonClient.AddonV1beta1().ClusterManagementAddOns().Update(context.Background(), old, metav1.UpdateOptions{}) - if err == nil { - return true - } - return false + _, err = hubAddonClient.AddonV1beta1().ClusterManagementAddOns().Update(context.Background(), old, metav1.UpdateOptions{}) + return err == nil }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) }
130-140: Same pattern as above—consider explicit error handling.Migration to v1beta1 is correct. Same optional improvement applies: check the
Geterror before accessingold.Status.🔎 Proposed improvement
func updateManagedClusterAddOnStatus(ctx context.Context, new *addonapiv1beta1.ManagedClusterAddOn) { gomega.Eventually(func() bool { old, err := hubAddonClient.AddonV1beta1().ManagedClusterAddOns(new.Namespace).Get(context.Background(), new.Name, metav1.GetOptions{}) + if err != nil { + return false + } old.Status = new.Status - _, err = hubAddonClient.AddonV1beta1().ManagedClusterAddOns(old.Namespace).UpdateStatus(context.Background(), old, metav1.UpdateOptions{}) - if err == nil { - return true - } - return false + _, err = hubAddonClient.AddonV1beta1().ManagedClusterAddOns(old.Namespace).UpdateStatus(context.Background(), old, metav1.UpdateOptions{}) + return err == nil }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) }pkg/addonmanager/addontesting/helpers.go (1)
139-154: Consider removing or documenting unused parameters.The
crdandcrparameters are declared but unused in the function body. These were likely used in v1alpha1 (possibly for AddOnConfiguration or AddOnMeta fields) but are no longer needed in v1beta1. Consider either removing them in a follow-up cleanup or renaming them to_to signal they're intentionally unused.🔎 Proposed signature cleanup
-func NewClusterManagementAddon(name, crd, cr string) *clusterManagementAddonBuilder { +func NewClusterManagementAddon(name string) *clusterManagementAddonBuilder { return &clusterManagementAddonBuilder{ &addonapiv1beta1.ClusterManagementAddOn{Note: This would require updating any test callers of this function.
pkg/addonmanager/constants/constants.go (1)
65-84: Good backward compatibility approach for manifest location.The fallback to a hardcoded label key (
"addon.open-cluster-management.io/hosted-manifest-location") on line 72 provides backward compatibility when the annotation is not set. Consider extracting this hardcoded string to a named constant for clarity and consistency with the other backward-compatibility constants defined above.🔎 Optional: Extract hardcoded label key to a constant
const ( // ...existing constants... + // HostedManifestLocationLabelKey is the deprecated label key for hosted manifest location. + // This was removed in v1beta1 in favor of annotation only. + HostedManifestLocationLabelKey = "addon.open-cluster-management.io/hosted-manifest-location" ) func GetHostedManifestLocation(labels, annotations map[string]string) (string, bool, error) { manifestLocation := annotations[addonv1beta1.HostedManifestLocationAnnotationKey] // In v1beta1, HostedManifestLocationLabelKey was removed in favor of annotation only // For backward compatibility, check labels if annotation is not set if manifestLocation == "" { - // Use the hardcoded label key for backward compatibility - manifestLocation = labels["addon.open-cluster-management.io/hosted-manifest-location"] + manifestLocation = labels[HostedManifestLocationLabelKey] }pkg/addonmanager/manager.go (1)
14-16: Consider renaming the alias for clarity.The alias
addonv1alpha1importsopen-cluster-management.io/api/addon/v1beta1, which is misleading. Consider renaming toaddonv1beta1oraddonapiv1beta1to match the actual API version being used.🔎 Proposed fix
- addonv1alpha1 "open-cluster-management.io/api/addon/v1beta1" + addonapiv1beta1 "open-cluster-management.io/api/addon/v1beta1"Then update the references at lines 70 and 85 to use the new alias.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (79)
cmd/example/helloworld/main.gocmd/example/helloworld_helm/main.goexamples/helloworld/helloworld.goexamples/helloworld/helloworld_test.goexamples/helloworld_agent/agent.goexamples/helloworld_helm/helloworld_helm.goexamples/helloworld_helm/helloworld_helm_test.goexamples/rbac/rbac.gopkg/addonfactory/addondeploymentconfig.gopkg/addonfactory/addondeploymentconfig_test.gopkg/addonfactory/addonfactory.gopkg/addonfactory/helm_agentaddon.gopkg/addonfactory/helm_agentaddon_test.gopkg/addonfactory/helper.gopkg/addonfactory/template_agentaddon.gopkg/addonfactory/template_agentaddon_test.gopkg/addonfactory/test_helper.gopkg/addonmanager/addontesting/helpers.gopkg/addonmanager/base_manager.gopkg/addonmanager/cloudevents/manager.gopkg/addonmanager/constants/constants.gopkg/addonmanager/controllers/addonconfig/controller.gopkg/addonmanager/controllers/addonconfig/controller_test.gopkg/addonmanager/controllers/agentdeploy/controller.gopkg/addonmanager/controllers/agentdeploy/default_hook_sync.gopkg/addonmanager/controllers/agentdeploy/default_hook_sync_test.gopkg/addonmanager/controllers/agentdeploy/default_sync.gopkg/addonmanager/controllers/agentdeploy/default_sync_test.gopkg/addonmanager/controllers/agentdeploy/healthcheck_sync.gopkg/addonmanager/controllers/agentdeploy/healthcheck_sync_test.gopkg/addonmanager/controllers/agentdeploy/hosted_hook_sync.gopkg/addonmanager/controllers/agentdeploy/hosted_hook_sync_test.gopkg/addonmanager/controllers/agentdeploy/hosted_sync.gopkg/addonmanager/controllers/agentdeploy/hosted_sync_test.gopkg/addonmanager/controllers/agentdeploy/util_test.gopkg/addonmanager/controllers/agentdeploy/utils.gopkg/addonmanager/controllers/certificate/csrapprove.gopkg/addonmanager/controllers/certificate/csrapprove_test.gopkg/addonmanager/controllers/certificate/csrsign.gopkg/addonmanager/controllers/certificate/csrsign_test.gopkg/addonmanager/controllers/cmaconfig/controller.gopkg/addonmanager/controllers/cmaconfig/controller_test.gopkg/addonmanager/controllers/cmamanagedby/controller.gopkg/addonmanager/controllers/cmamanagedby/controller_test.gopkg/addonmanager/controllers/registration/controller.gopkg/addonmanager/controllers/registration/controller_test.gopkg/addonmanager/manager.gopkg/agent/inteface.gopkg/index/index.gopkg/utils/addon.gopkg/utils/addon_config.gopkg/utils/addon_config_test.gopkg/utils/addon_test.gopkg/utils/csr_helper_test.gopkg/utils/csr_helpers.gopkg/utils/helpers.gopkg/utils/helpers_test.gopkg/utils/permission.gopkg/utils/permission_test.gopkg/utils/probe_helper.gotest/e2e/helloworld_helm_test.gotest/e2e/helloworld_test.gotest/e2ehosted/helloworld_hosted_test.gotest/integration/cloudevents/agent_deploy_test.gotest/integration/cloudevents/agent_hook_deploy_test.gotest/integration/cloudevents/agent_hosting_deploy_test.gotest/integration/cloudevents/agent_hosting_hook_deploy_test.gotest/integration/cloudevents/suite_test.gotest/integration/kube/agent_deploy_test.gotest/integration/kube/agent_hook_deploy_test.gotest/integration/kube/agent_hosting_deploy_test.gotest/integration/kube/agent_hosting_hook_deploy_test.gotest/integration/kube/assertion_test.gotest/integration/kube/cluster_management_addon_test.gotest/integration/kube/csr_test.gotest/integration/kube/deleted_managed_cluster_create_addon_test.gotest/integration/kube/multiworks_test.gotest/integration/kube/registration_test.gotest/integration/kube/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/addonmanager/cloudevents/manager.go
- pkg/addonfactory/template_agentaddon_test.go
- pkg/utils/csr_helpers.go
- pkg/addonfactory/test_helper.go
- cmd/example/helloworld/main.go
- pkg/utils/addon.go
- test/integration/kube/registration_test.go
- examples/rbac/rbac.go
- cmd/example/helloworld_helm/main.go
- pkg/utils/permission.go
- pkg/utils/csr_helper_test.go
- pkg/addonmanager/controllers/agentdeploy/default_sync.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T08:14:41.882Z
Learnt from: zhujian7
Repo: open-cluster-management-io/addon-framework PR: 334
File: pkg/addonmanager/base_manager.go:83-87
Timestamp: 2025-09-15T08:14:41.882Z
Learning: In template-based addon mode (templateBasedAddOn=true), the FilterTemplateBasedAddOns function intentionally returns false for template-based addons whose Status.ConfigReferences are not yet populated with the proper AddOnTemplate reference and SpecHash. This ensures controllers retry and wait for template configuration to be fully established before processing, preventing premature processing of incompletely configured template addons.
Applied to files:
pkg/addonmanager/controllers/addonconfig/controller_test.gopkg/utils/addon_test.gopkg/addonmanager/controllers/agentdeploy/utils.gotest/integration/cloudevents/agent_deploy_test.gopkg/utils/addon_config.gotest/integration/kube/agent_hosting_hook_deploy_test.gotest/integration/cloudevents/agent_hosting_hook_deploy_test.go
📚 Learning: 2025-09-15T09:30:55.917Z
Learnt from: zhujian7
Repo: open-cluster-management-io/addon-framework PR: 334
File: pkg/addonmanager/base_manager.go:107-111
Timestamp: 2025-09-15T09:30:55.917Z
Learning: BaseAddonManagerImpl instances are designed to operate in a single mode: either templateBasedAddOn=true (handles only template-based addons) or templateBasedAddOn=false (handles all addons). A single manager instance does not handle mixed addon types - this architectural separation ensures clean lifecycle management.
Applied to files:
pkg/addonmanager/base_manager.go
🧬 Code graph analysis (19)
test/integration/kube/agent_hosting_deploy_test.go (1)
pkg/addonmanager/constants/constants.go (1)
InstallModeHosted(14-14)
examples/helloworld/helloworld.go (1)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)
examples/helloworld_helm/helloworld_helm.go (1)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)
pkg/addonmanager/base_manager.go (2)
pkg/addonmanager/controllers/cmaconfig/controller.go (1)
NewCMAConfigController(47-79)pkg/addonmanager/controllers/certificate/csrsign.go (1)
NewCSRSignController(40-81)
pkg/addonmanager/controllers/registration/controller.go (2)
pkg/agent/inteface.go (1)
AgentAddon(19-36)pkg/utils/addon.go (1)
ManagedClusterAddOnFilterFunc(10-10)
pkg/addonfactory/helper.go (1)
pkg/addonfactory/addonfactory.go (1)
Values(25-25)
pkg/addonmanager/controllers/agentdeploy/utils.go (1)
pkg/agent/inteface.go (1)
AgentAddon(19-36)
test/integration/kube/suite_test.go (2)
pkg/addonmanager/constants/constants.go (1)
GetHostedModeInfo(52-62)pkg/agent/inteface.go (1)
HealthProber(172-176)
test/integration/cloudevents/agent_deploy_test.go (1)
pkg/utils/helpers.go (1)
MergeOwnerRefs(189-225)
pkg/utils/addon_config.go (1)
pkg/addonfactory/addondeploymentconfig.go (1)
NewAddOnDeploymentConfigGetter(195-197)
pkg/addonmanager/controllers/cmamanagedby/controller_test.go (3)
pkg/addonmanager/constants/constants.go (2)
AddonLifecycleAnnotationKey(20-20)AddonLifecycleSelfManageAnnotationValue(28-28)pkg/addonmanager/addontesting/helpers.go (1)
AssertActions(297-306)pkg/utils/helpers.go (1)
FilterByAddonName(311-317)
pkg/addonmanager/controllers/certificate/csrsign_test.go (1)
pkg/agent/inteface.go (2)
AgentAddonOptions(39-99)RegistrationOption(119-162)
pkg/addonmanager/controllers/registration/controller_test.go (2)
pkg/addonmanager/addontesting/helpers.go (1)
NewAddon(81-89)pkg/agent/inteface.go (1)
AgentAddon(19-36)
pkg/addonfactory/addondeploymentconfig_test.go (1)
pkg/addonmanager/addontesting/helpers.go (1)
NewAddon(81-89)
pkg/addonmanager/controllers/cmaconfig/controller_test.go (1)
pkg/addonmanager/addontesting/helpers.go (1)
NewClusterManagementAddon(139-154)
test/integration/cloudevents/suite_test.go (2)
pkg/addonmanager/constants/constants.go (1)
GetHostedModeInfo(52-62)pkg/agent/inteface.go (1)
HealthProber(172-176)
test/integration/kube/assertion_test.go (2)
pkg/addonmanager/constants/constants.go (2)
AddonLifecycleAnnotationKey(20-20)AddonLifecycleAddonManagerAnnotationValue(24-24)pkg/utils/helpers.go (1)
MergeOwnerRefs(189-225)
pkg/addonfactory/helm_agentaddon.go (1)
pkg/addonfactory/addonfactory.go (2)
Values(25-25)AddonDefaultInstallNamespace(20-20)
examples/helloworld/helloworld_test.go (1)
pkg/addonmanager/addontesting/helpers.go (1)
NewAddon(81-89)
🪛 GitHub Actions: CloudEventsIntegration
test/integration/cloudevents/agent_hosting_deploy_test.go
[error] 142-142: the server could not find the requested resource (post clustermanagementaddons.addon.open-cluster-management.io)
⏰ 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). (5)
- GitHub Check: unit
- GitHub Check: e2e-cloudevents
- GitHub Check: e2e-hosted
- GitHub Check: verify
- GitHub Check: e2e
| fakeAddonClient := fakeaddon.NewSimpleClientset(c.cmas...) | ||
| addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute) | ||
| addonInformer := addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer() | ||
| addonInformer := addonInformers.Addon().V1beta1().ManagedClusterAddOns().Informer() |
There was a problem hiding this comment.
Critical: Incorrect informer type causes type mismatch.
Line 349 creates an informer for ManagedClusterAddOns, but the test logic expects ClusterManagementAddOns:
- The field name is
clusterManagementAddonIndexer(line 352) - The indexer uses
index.ClusterManagementAddonByConfig(line 356) - The enqueue function is
enqueueClusterManagementAddOnsByConfig(line 366) - Test cases create and add
ClusterManagementAddOnobjects to the store (lines 297-363)
This type mismatch will cause the test to fail or behave incorrectly.
🔎 Fix: Use ClusterManagementAddOns informer
- addonInformer := addonInformers.Addon().V1beta1().ManagedClusterAddOns().Informer()
+ addonInformer := addonInformers.Addon().V1beta1().ClusterManagementAddOns().Informer()🤖 Prompt for AI Agents
In pkg/addonmanager/controllers/cmaconfig/controller_test.go around line 349,
the test creates an informer for ManagedClusterAddOns but the test logic and
fixtures expect ClusterManagementAddOns; replace the ManagedClusterAddOns
informer with the ClusterManagementAddOns informer (use
addonInformers.Addon().V1beta1().ClusterManagementAddOns().Informer()), update
the variable name if necessary to reflect
clusterManagementAddonInformer/indexer, and ensure any related type
assertions/imports reference ClusterManagementAddOn so the indexer, enqueue
function and test objects match the informer type.
| deployConfig := &addonapiv1alpha1.AddOnDeploymentConfig{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-deploy-config", | ||
| Namespace: managedClusterName, | ||
| }, | ||
| Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{ | ||
| AgentInstallNamespace: "default", | ||
| }, | ||
| } | ||
| _, err = hubAddonClient.AddonV1alpha1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) |
There was a problem hiding this comment.
Critical: AddOnDeploymentConfig should use v1beta1 API.
This code creates an AddOnDeploymentConfig using addonapiv1alpha1 types and calls hubAddonClient.AddonV1alpha1(), which is inconsistent with the v1beta1 migration. All addon resources in this test should use v1beta1 APIs for consistency.
🔎 Proposed fix to use v1beta1 AddOnDeploymentConfig
- deployConfig := &addonapiv1alpha1.AddOnDeploymentConfig{
+ deployConfig := &addonapiv1beta1.AddOnDeploymentConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "test-deploy-config",
Namespace: managedClusterName,
},
- Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{
+ Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{
AgentInstallNamespace: "default",
},
}
- _, err = hubAddonClient.AddonV1alpha1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{})
+ _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())📝 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.
| deployConfig := &addonapiv1alpha1.AddOnDeploymentConfig{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "test-deploy-config", | |
| Namespace: managedClusterName, | |
| }, | |
| Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{ | |
| AgentInstallNamespace: "default", | |
| }, | |
| } | |
| _, err = hubAddonClient.AddonV1alpha1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | |
| deployConfig := &addonapiv1beta1.AddOnDeploymentConfig{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "test-deploy-config", | |
| Namespace: managedClusterName, | |
| }, | |
| Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{ | |
| AgentInstallNamespace: "default", | |
| }, | |
| } | |
| _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(managedClusterName).Create(context.Background(), deployConfig, metav1.CreateOptions{}) | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) |
🤖 Prompt for AI Agents
In test/integration/cloudevents/agent_hook_deploy_test.go around lines 113 to
123, the test constructs and creates an AddOnDeploymentConfig using the v1alpha1
types and hubAddonClient.AddonV1alpha1(), but the suite was migrated to v1beta1;
replace usage with the v1beta1 API: change the type to the addonapiv1beta1
AddOnDeploymentConfig, update any import alias to addonapiv1beta1, and call
hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(managedClusterName).Create(...);
ensure the object fields remain the same and update any compile-time
references/imports accordingly.
1 participant
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.