-
Notifications
You must be signed in to change notification settings - Fork 40
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
🌱 Add a method to set the hosting cluster for the Helm agent #246
🌱 Add a method to set the hosting cluster for the Helm agent #246
Conversation
The policy addons require setting this since we need the hosting cluster capabilities variables and we can't guarantee that the hosting cluster is static for each addon. Signed-off-by: mprahl <mprahl@users.noreply.github.com>
@@ -219,6 +219,10 @@ func (a *HelmAgentAddon) getBuiltinValues( | |||
return helmBuiltinValues, nil | |||
} | |||
|
|||
func (a *HelmAgentAddon) SetHostingCluster(hostingCluster *clusterv1.ManagedCluster) { |
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.
Hi @mprahl, there is a WithHostingCluster method for agentAddonFactory already, not sure if that is what you need?
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.
@zhujian7 the issue is that WithHostingCluster
forces a single value for all hosted clusters, however, the hosting cluster is set at the ManagedClusterAddOn
object level, so it could be different hosting clusters for different hosted clusters for the same addon.
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, you are right, but I am still confused about how the func SetHostingCluster
to the HelmAgentAddon
can set different hosting clusters for different hosted clusters for the same addon, could you give some examples/tests of how to use the added SetHostingCluster
for addon developers?
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.
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.
emmm... but setting hostingCluster in the Manifests
method is weird.
@zhiweiyin318 @qiujian16 @haoqing0110 WDYT? any suggested way to implement this?
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.
@zhujian7 I'm open to other ideas but in #149, the maintainers said that changing the method signatures by adding additional arguments was a no go.
Another option is to have Manifests
in the addon framework query for the hosting cluster if set on the addon like we do in our code rather than have it as a struct field.
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.
I am think: can we remove the hostingCluster
field for struct HelmAgentAddon
, and not use if a.hostingCluster != nil
here, instead we get the hostingcluster name from managedClusterAddon.annotations["addon.open-cluster-management.io/hosting-cluster-name"]
, and then use a hub clusterCient to get the hostingcluster.
The main reason I think this is better is: from my point of view the HelmAgentAddon
is used for config the addon for all managed clusters(it is like per clustermanagementaddon), but the hostingcluster is for one managed cluster(per managedclusteraddon), so it should not be a field of the HelmAgentAddon
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.
@zhujian7 , yes, I agree but I don't have the capacity to do this refactoring right now. If you do it, I'd be happy to adopt that change.
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.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mprahl, zhiweiyin318 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
68f628e
into
open-cluster-management-io:main
The policy addons require setting this since we need the hosting cluster capabilities variables and we can't guarantee that the hosting cluster is static for each addon.