-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Define install strategy in ClusterManagementAddon #112
base: main
Are you sure you want to change the base?
✨ Define install strategy in ClusterManagementAddon #112
Conversation
Signed-off-by: Rokibul Hasan <mdrokibulhasan@appscode.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RokibulHasan7 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 |
/assign @zhujian7 |
@@ -21,3 +19,8 @@ spec: | |||
defaultConfig: | |||
name: {{ .Values.addOnTemplateName | default (print "managed-serviceaccount-" .Chart.Version) }} | |||
{{- end }} | |||
installStrategy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also control whether to install the agent by installStrategy or not? like the.Values.agentInstallAll
did?
So users who want to install the agent by themselves are not affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, .Values.agentInstallAll
is using WithInstallStrategy
from the addon framework. However, this approach is deprecated, and it is recommended to use the install strategy in ClusterManagementAddon
instead. Is there any other way to install agent on all managed clusters using the addon framework?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the install strategy in ClusterManagementAddon is the recommended way, no other way to install agent using the addon framework. But users should be able to decide whether to use the install strategy or install agent by themselves(create a managedclusteraddon CR in the target cluster namespace)
agentFactory.WithInstallStrategy(agent.InstallAllStrategy(common.AddonAgentInstallNamespace)) | ||
} | ||
WithAgentDeployTriggerClusterFilter(utils.ClusterImageRegistriesAnnotationChanged). | ||
WithAgentInstallNamespace(func(addon *v1alpha1.ManagedClusterAddOn) string { return common.AddonAgentInstallNamespace }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return common.AddonAgentInstallNamespace
will make the ns not configurable, I recommend we read the ns from AddonDeploymentConfig first, if it is not configured by an addondeploymentconfig, then we return the default common.AddonAgentInstallNamespac
e value.
and there are some helper funcs you can use in the addon-framework repo. like https://github.com/open-cluster-management-io/addon-framework/blob/5c9527506035ef7a81c03af45e3dbc1597645fbc/cmd/example/helloworld_helm/main.go#L131C1-L134C6
@@ -0,0 +1,10 @@ | |||
{{- if not .Values.installByPlacement.placementName }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I am not sure if the placement and clustersetbinding are needed, maybe it is clearer that the placement is created/controlled by other components. And here we just use the results(which clusters are selected).
@qiujian16 WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think we do not need to include placement here.
Signed-off-by: red-hat-konflux <123456+red-hat-konflux[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com>
Summary
Related issue(s)
Fixes #