refactor(kubernetes): force usage of Derived kubernetes #125
Conversation
Prevents consumers of the kubernetes package the usage of public methods on a non-derived config instance.
pkg/kubernetes/kubernetes.go
Outdated
| IsOpenShift(ctx context.Context) bool | ||
| } | ||
|
|
||
| type DerivedKubernetes interface { |
There was a problem hiding this comment.
This interface is too rigid to be used to implement different DerivedKubernetes objects. In my opinion, having a struct is better, probably after renaming the Kubernetes (which hides the package names) to KubernetesManager.
pkg/kubernetes/kubernetes.go
Outdated
| type CloseWatchKubeConfig func() error | ||
|
|
||
| type Kubernetes struct { | ||
| type Kubernetes interface { |
There was a problem hiding this comment.
Same, I don't think we need this for now
pkg/kubernetes/kubernetes.go
Outdated
| ResourcesDelete(ctx context.Context, gvk *schema.GroupVersionKind, namespace, name string) error | ||
| } | ||
|
|
||
| type kubernetes struct { |
There was a problem hiding this comment.
I'd recommend renaming it to KubernetesManager. I don't think, exporting the fields in here hurts us. However, in my opinion, maybe we can get rid of some of them (e.g. I bet we don't need parameterCodec by using an upstream functionality.).
pkg/kubernetes/kubernetes.go
Outdated
| return derived | ||
| } | ||
|
|
||
| func (k *kubernetes) CacheInvalidate() { |
There was a problem hiding this comment.
Why do we need invalidating the cache?. Doesn't MCP server interact with the same cluster throughout its lifetime?. Stdio would require this. But maybe when stdio is used, we fully use a different path.
There was a problem hiding this comment.
This is currently used for testing purposes only.
In this case, the cache becomes stale because the metrics-api is made available after an initial check.
As stated, this isn't used as of now in production code, but I assume that similar circumstances might happen in a real cluster (cluster is the same, available resources might change).
There was a problem hiding this comment.
DiscoveryClient has etag mechanism that compares the cached hash values against the API Server each time. If it detects any change in etags, it internally invalidates the cache.
There was a problem hiding this comment.
We should check that together, the test doesn't validate that behavior (I think). (I'll add a TODO)
There was a problem hiding this comment.
Yeah, this invalidation mechanism requires to have an actual API server. So I wouldn't be surprised if unit tests fails due to this. Would it be an issue to add this invalidation in unit test?.
There was a problem hiding this comment.
kubernetes/enhancements#3352 in case you are interested in
There was a problem hiding this comment.
The test using this is here: https://github.com/manusa/kubernetes-mcp-server/blob/54d564a54e38febc63c56c60b9ccb76501adf55f/pkg/mcp/pods_top_test.go#L10
There's a fake API server behind which does reply. Only one request is performed.
I'll set the TODO and we can check later
|
@ardaguclu, please check if the subsequent commit aligns with your expectations |
manusa
force-pushed
the
refactor/force-derived
branch
from
d67ac47 to
56576c1
Compare
Addresses comment by ardaguclu
manusa
force-pushed
the
refactor/force-derived
branch
from
56576c1 to
a5cd799
Compare
|
This looks good to me |
Prevents consumers of the kubernetes package the usage of public methods on a non-derived config instance.
/cc @ardaguclu