✨ Make install hubaddons by helm chart more general#533
✨ Make install hubaddons by helm chart more general#533qiujian16 wants to merge 1 commit intoopen-cluster-management-io:mainfrom
Conversation
|
/hold |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughReplaces hard-coded addon identifiers with a public AddonChart map and exported addon name constants; introduces installPolicyAddon/uninstallPolicy flows; removes Values.HubAddons; aggregates install validation errors; adapts Helm client flows to use AddonCharts; updates tests to the new policy-addon-focused workflow. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cmd/install/hubaddon/exec_test.go (1)
42-50:⚠️ Potential issue | 🟡 MinorTest may be passing for the wrong reason.
The test description states it validates "not a valid add-on name", but the
validate()function inexec.gono longer validates addon names—it only checks thatnamesis non-empty and that the version bundle is valid. This test likely passes becausebundleVersionis empty (not set in the Options), causingGetVersionBundleto fail, not because of the invalid addon name.Consider either:
- Updating the test description to accurately reflect what it's testing
- Or, if addon name validation is desired, implementing it in
validate()using theaddonChartsmap orscenario.AddonDeploymentFiles
🤖 Fix all issues with AI agents
In `@pkg/cmd/install/hubaddon/exec.go`:
- Line 126: The printed status "Installing built-in %s add-on..."
(fmt.Fprintf(o.Streams.Out, ... , policyFrameworkAddonName)) is emitted after
CRDs/configs/deployments are applied, which is misleading; move that fmt.Fprintf
call to execute before the sequence of Apply calls that apply CRDs, configs, and
deployments (the Apply.../apply functions in this file) so the message appears
when installation begins rather than after completion.
🧹 Nitpick comments (3)
pkg/cmd/install/hubaddon/exec.go (2)
33-51: Unusedversionfield inaddonChartstruct.The
versionfield defined on line 37 is never used anywhere in the code. If this is intended for future use, consider adding a comment or TODO. Otherwise, remove it to avoid confusion.♻️ Proposed fix to remove unused field
type addonChart struct { chartName string releaseName string namespace string - version string }
168-176: Fallback chart configuration uses addon name directly.The fallback for unknown addons (lines 171-175) assumes the chart name and release name match the addon name exactly. This may not always be true for all Helm charts in the OCM repository.
Consider adding a warning or debug log when using the fallback to help users diagnose issues if their addon installation fails due to chart naming mismatches.
♻️ Proposed improvement
addonChartToInstall, ok := addonCharts[addon] if !ok { + klog.V(2).InfoS("No explicit chart configuration found, using addon name as defaults", "addon", addon) addonChartToInstall = addonChart{ chartName: addon, releaseName: addon, namespace: o.values.Namespace, } }pkg/cmd/install/hubaddon/exec_test.go (1)
72-72: MissingInstream in IOStreams.The
Streamsfield is initialized withOutandErrOutbut notIn. While likely not needed for this test, it's good practice to initialize all fields ofIOStreamsfor completeness, especially if the code under test ever reads from stdin.♻️ Proposed fix
- Streams: genericiooptions.IOStreams{Out: os.Stdout, ErrOut: os.Stderr}, + Streams: genericiooptions.IOStreams{In: os.Stdin, Out: os.Stdout, ErrOut: os.Stderr},
pkg/cmd/install/hubaddon/exec.go
Outdated
There was a problem hiding this comment.
User-facing message printed after installation completes.
The "Installing built-in..." message is printed after CRDs, configs, and deployments have already been applied (lines 113-124). This is misleading since it suggests the installation is starting when it has already finished. Move this message before the Apply calls for accurate progress feedback.
🛠️ Proposed fix
func (o *Options) installPolicyAddon() error {
if o.values.CreateNamespace {
if err := o.createNamespace(); err != nil {
return err
}
}
+ fmt.Fprintf(o.Streams.Out, "Installing built-in %s add-on to the Hub cluster...\n", policyFrameworkAddonName)
+
r := reader.NewResourceReader(o.ClusteradmFlags.KubectlFactory, o.ClusteradmFlags.DryRun, o.Streams)
files, ok := scenario.AddonDeploymentFiles[policyFrameworkAddonName]
if !ok {
return fmt.Errorf("no policy framework addon")
}
err := r.Apply(scenario.Files, o.values, files.CRDFiles...)
if err != nil {
return fmt.Errorf("Error deploying %s CRDs: %w", policyFrameworkAddonName, err)
}
err = r.Apply(scenario.Files, o.values, files.ConfigFiles...)
if err != nil {
return fmt.Errorf("Error deploying %s dependencies: %w", policyFrameworkAddonName, err)
}
err = r.Apply(scenario.Files, o.values, files.DeploymentFiles...)
if err != nil {
return fmt.Errorf("Error deploying %s deployments: %w", policyFrameworkAddonName, err)
}
- fmt.Fprintf(o.Streams.Out, "Installing built-in %s add-on to the Hub cluster...\n", policyFrameworkAddonName)
-
if len(o.outputFile) > 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.
| fmt.Fprintf(o.Streams.Out, "Installing built-in %s add-on to the Hub cluster...\n", policyFrameworkAddonName) | |
| func (o *Options) installPolicyAddon() error { | |
| if o.values.CreateNamespace { | |
| if err := o.createNamespace(); err != nil { | |
| return err | |
| } | |
| } | |
| fmt.Fprintf(o.Streams.Out, "Installing built-in %s add-on to the Hub cluster...\n", policyFrameworkAddonName) | |
| r := reader.NewResourceReader(o.ClusteradmFlags.KubectlFactory, o.ClusteradmFlags.DryRun, o.Streams) | |
| files, ok := scenario.AddonDeploymentFiles[policyFrameworkAddonName] | |
| if !ok { | |
| return fmt.Errorf("no policy framework addon") | |
| } | |
| err := r.Apply(scenario.Files, o.values, files.CRDFiles...) | |
| if err != nil { | |
| return fmt.Errorf("Error deploying %s CRDs: %w", policyFrameworkAddonName, err) | |
| } | |
| err = r.Apply(scenario.Files, o.values, files.ConfigFiles...) | |
| if err != nil { | |
| return fmt.Errorf("Error deploying %s dependencies: %w", policyFrameworkAddonName, err) | |
| } | |
| err = r.Apply(scenario.Files, o.values, files.DeploymentFiles...) | |
| if err != nil { | |
| return fmt.Errorf("Error deploying %s deployments: %w", policyFrameworkAddonName, err) | |
| } | |
| if len(o.outputFile) > 0 { |
🤖 Prompt for AI Agents
In `@pkg/cmd/install/hubaddon/exec.go` at line 126, The printed status "Installing
built-in %s add-on..." (fmt.Fprintf(o.Streams.Out, ... ,
policyFrameworkAddonName)) is emitted after CRDs/configs/deployments are
applied, which is misleading; move that fmt.Fprintf call to execute before the
sequence of Apply calls that apply CRDs, configs, and deployments (the
Apply.../apply functions in this file) so the message appears when installation
begins rather than after completion.
qiujian16
force-pushed
the
install-addon-by-helm
branch
from
cb11547 to
69f464b
Compare
Signed-off-by: Jian Qiu <jqiu@redhat.com>
qiujian16
force-pushed
the
install-addon-by-helm
branch
from
69f464b to
8b7bda1
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cmd/uninstall/hubaddon/exec.go (1)
4-49:⚠️ Potential issue | 🟠 MajorOnly the first addon is uninstalled (set order is nondeterministic).
The loop returns on the first element, so additional addons are skipped and the selection depends on set iteration order. Iterate all addons and aggregate errors (similar to install).
🛠️ Proposed fix
import ( "fmt" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "strings" @@ func (o *Options) run() error { addons := sets.New[string](strings.Split(o.names, ",")...) if len(addons) == 0 { return nil } - for a := range addons { - if a == clusteradmhubaddoninstall.PolicyFrameworkAddonName { - return o.uninstallPolicy() - } - return o.runWithHelmClient(a) - } - - return o.uninstallPolicy() + var errs []error + for a := range addons { + if a == clusteradmhubaddoninstall.PolicyFrameworkAddonName { + if err := o.uninstallPolicy(); err != nil { + errs = append(errs, err) + } + continue + } + if err := o.runWithHelmClient(a); err != nil { + errs = append(errs, err) + } + } + return utilerrors.NewAggregate(errs) }
🤖 Fix all issues with AI agents
In `@pkg/cmd/install/hubaddon/exec_test.go`:
- Line 23: The test is expecting validate() to reject invalidAddon
("no-such-addon") but Options.validate now only checks non-empty names and
bundle version parsing; either update the test to match the new behavior by
removing the expectation of an error for invalidAddon (or asserting that
validate() succeeds for that input), or reintroduce addon-name validation inside
Options.validate (or a helper it calls) to explicitly check the name against the
canonical AddonCharts list and any policy rules before returning; locate the
test's use of invalidAddon in exec_test.go and the Options.validate
implementation to apply the corresponding change so the test and validate()
behavior stay consistent.
- Around line 78-88: The readiness check currently compares
appDeployment.Status.AvailableReplicas to appDeployment.Status.Replicas which
can be lower than the desired replica count; update the gomega.Eventually
closure that iterates policyAddonDeployments (the
kubeClient.AppsV1().Deployments(...).Get call) to use the desired replica count
from appDeployment.Spec.Replicas (treat nil as default 1 or 0 per your
convention) as the expected value instead of Status.Replicas so the test only
passes when AvailableReplicas equals the intended Spec.Replicas.
In `@pkg/cmd/uninstall/hubaddon/exec.go`:
- Around line 61-63: The code currently ignores errors from
version.GetVersionBundle when setting o.values.BundleVersion; instead call
version.GetVersionBundle("default", "") and check the returned error, returning
it (or wrapping it) from the enclosing function rather than discarding it so
uninstall does not proceed with an invalid BundleVersion; update the logic
around o.values.BundleVersion assignment in exec.go to handle and propagate the
error from GetVersionBundle.
- Around line 81-90: The uninstall always sets Helm namespace to
o.values.Namespace causing releases in addon-specific namespaces to be missed;
update the code around addonChart lookup (AddonCharts and addonChart) to choose
the namespace: use addonChart.Namespace when it is non-empty, otherwise fall
back to o.values.Namespace, then call o.Helm.WithNamespace(chosenNamespace)
before o.Helm.UninstallRelease(addonChart.ReleaseName); ensure any default
AddonChart created also allows its Namespace to be empty so the fallback works.
| for _, deployment := range policyAddonDeployments { | ||
| gomega.Eventually(func() (bool, error) { | ||
| appDeployment, err := kubeClient.AppsV1().Deployments(ocmNamespace).Get(context.Background(), deployment, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| availableReplicas := appDeployment.Status.AvailableReplicas | ||
| expectedReplicas := appDeployment.Status.Replicas | ||
| return availableReplicas == expectedReplicas, nil | ||
| }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue(), deployment+" deployment should be ready") |
There was a problem hiding this comment.
Readiness check can pass before desired replicas are reached.
Status.Replicas can be less than desired; using it can yield a false positive when AvailableReplicas == Status.Replicas but Spec.Replicas is higher. Use Spec.Replicas (with nil default) as the expected count.
🛠️ Proposed fix
- expectedReplicas := appDeployment.Status.Replicas
- return availableReplicas == expectedReplicas, nil
+ expectedReplicas := int32(1)
+ if appDeployment.Spec.Replicas != nil {
+ expectedReplicas = *appDeployment.Spec.Replicas
+ }
+ return availableReplicas == expectedReplicas, nil🤖 Prompt for AI Agents
In `@pkg/cmd/install/hubaddon/exec_test.go` around lines 78 - 88, The readiness
check currently compares appDeployment.Status.AvailableReplicas to
appDeployment.Status.Replicas which can be lower than the desired replica count;
update the gomega.Eventually closure that iterates policyAddonDeployments (the
kubeClient.AppsV1().Deployments(...).Get call) to use the desired replica count
from appDeployment.Spec.Replicas (treat nil as default 1 or 0 per your
convention) as the expected value instead of Status.Replicas so the test only
passes when AvailableReplicas equals the intended Spec.Replicas.
| // this needs to be set to render the manifests, but the version value | ||
| // does not matter. | ||
| o.values.BundleVersion, _ = version.GetVersionBundle("default", "") |
There was a problem hiding this comment.
Don’t ignore GetVersionBundle errors.
If the default bundle cannot be resolved, uninstall proceeds with an invalid BundleVersion. Return the error instead.
🛠️ Proposed fix
- o.values.BundleVersion, _ = version.GetVersionBundle("default", "")
+ bundle, err := version.GetVersionBundle("default", "")
+ if err != nil {
+ return err
+ }
+ o.values.BundleVersion = bundle📝 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.
| // this needs to be set to render the manifests, but the version value | |
| // does not matter. | |
| o.values.BundleVersion, _ = version.GetVersionBundle("default", "") | |
| // this needs to be set to render the manifests, but the version value | |
| // does not matter. | |
| bundle, err := version.GetVersionBundle("default", "") | |
| if err != nil { | |
| return err | |
| } | |
| o.values.BundleVersion = bundle |
🤖 Prompt for AI Agents
In `@pkg/cmd/uninstall/hubaddon/exec.go` around lines 61 - 63, The code currently
ignores errors from version.GetVersionBundle when setting
o.values.BundleVersion; instead call version.GetVersionBundle("default", "") and
check the returned error, returning it (or wrapping it) from the enclosing
function rather than discarding it so uninstall does not proceed with an invalid
BundleVersion; update the logic around o.values.BundleVersion assignment in
exec.go to handle and propagate the error from GetVersionBundle.
mikeshng
left a comment
mikeshng
left a comment
There was a problem hiding this comment.
The Argo CD related changes look good to me.
I will leave the lgtm to the policy experts.
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
Refactor
Chores