refactor(kubernetes): keep Provider as only external Kubernetes interface #372
Conversation
manusa
force-pushed
the
fix/kubernetes-provider
branch
2 times, most recently
from
51a65fe to
e355e10
Compare
| func (p *kubeConfigClusterProvider) IsOpenShift(ctx context.Context) bool { | ||
| return p.managers[p.defaultContext].IsOpenShift(ctx) | ||
| } |
There was a problem hiding this comment.
I'm not convinced is the IsOpenShift method makes sense here - what if there are a mix of openshift and non openshift clusters in the kubeconfig?
There was a problem hiding this comment.
Maybe we can either:
- check if any of the clusters are openshift
- figure out a way to set the "contexts" parameter for the openshift specific tools to only include the contexts which are openshift
There was a problem hiding this comment.
This is part of the legacy behavior where the Kubernetes MCP server is supposed to adapt to the underlying Kubernetes clusters and provide flavor-specific tools.
In the current state, the toolsets GetTools method receive this interface and can perform some logic to register tools based on the provided information.
With the Multi-Cluster approach and the segregation of toolsets, this might not be applicable any more.
I was going to add a TODO comment in the Provider interface definition beside the OpenShift reference indicating that this was going to be removed.
But since I'm not sure of how we want to move forward, I didn't.
Maybe we can either:
- check if any of the clusters are openshift
- figure out a way to set the "contexts" parameter for the openshift specific tools to only include the contexts which are openshift
These are valid options, the other alternative is to just remove the interface and provide a base openshift toolset.
Since it's unclear, and there are a few more refactors pending from my side, I think that the // TODO comment would be enough for now.
Thoughts?
There was a problem hiding this comment.
Yeah that makes sense - if we have a comment tracking that we should revisit this logic I'm happy to leave it as is for now
There was a problem hiding this comment.
I've added the TODO comment so that we eventually deal with this.
|
|
||
| // derive the manager based on auth on top of the settings for the cluster | ||
| k, err := m.Derived(ctx) | ||
| k, err := s.p.GetDerivedKubernetes(ctx, cluster) |
manusa
force-pushed
the
fix/kubernetes-provider
branch
from
a7dc685 to
68a2d71
Compare
manusa
force-pushed
the
fix/kubernetes-provider
branch
from
68a2d71 to
6a58d29
Compare
…face Initial phase to unify-merge the Provider interface with the Manager struct. - Renamed ManagerProvider to Provider (i.e. kubernets.Provider) - Moved Manager related logic to specific files - Exposed relevant method through Provider interface (GetDerivedKubernetes, IsOpenShift, VerifyToken) Signed-off-by: Marc Nuri <marc@marcnuri.com>
Co-authored-by: Calum Murray <cmurray@redhat.com> Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa
force-pushed
the
fix/kubernetes-provider
branch
from
6a58d29 to
f3f9fd8
Compare
2 participants
Initial phase to unify-merge the Provider interface with the Manager struct.
/cc @Cali0707