Bump k8s packages from 0.23.6 to 0.24.0 #10928
Conversation
This required modifying the `kube.Factory` interface to conform to changes in k8s' `cmdutil.Factory` interface: kubernetes/kubernetes@fe37728 Signed-off-by: Andrew Seigner <andrew@sig.gy>
pull-request-size
bot
added
the
size/S
|
|
||
| // DynamicClient returns a dynamic client ready for use | ||
| DynamicClient() (dynamic.Interface, error) |
There was a problem hiding this comment.
The Helm HIP that covers backwards compatibility refers to the Go project stance and direction on this which is that modifying interfaces is backward compatibility breaking. So, we add new interfaces for these cases. Could you please put them on a new interface and use the method in the linked Go docs to work with the new interface.
There was a problem hiding this comment.
@mattfarina Thanks for the review and links. I'm a bit unsure on the best way forward. I assume Helm's kube.Factory interface was built to mimic/wrap k8s' cmdutil.Factory. In v0.24.0, cmdutil.Factory's Validator method signature was modified. For kube.Factory to maintain backwards compatibility, we must continue supporting the older Validator signature, which no longer exists in k8s/k8s. To do this, I've introduced a NewFactoryShim() helper, that takes a cmdutil.Factory and returns a kube.Factory.
So a consumer that was previously doing this:
&kube.Client{
Factory: cmdutil.NewFactory(getter),
}... now does this:
&Client{
Factory: NewFactoryShim(cmdutil.NewFactory(getter)),
}Here is the full implementation:
https://github.com/siggy/helm/compare/main...siggy:siggy/k8s-v0-24-0-shim
An alternate (arguably less backwards-compatible) approach, would be to simply remove kube.Factory in favor of cmdutil.Factory. This approach would require no code changes for the consumer in the example above:
https://github.com/siggy/helm/compare/main...siggy:siggy/k8s-v0-24-0-remove
This all perhaps boils down to whether we break consumers that are implementing a kube.Factory directly, vs. breaking those that call cmdutil.NewFactory(getter) to get a cmdutil.Factory. In my own project, we're doing the latter.
There was a problem hiding this comment.
....aaand I just saw your subsequent review comments after posting this. Sounds like we're converging on an approach. Standing by.
| // Returns a schema that can validate objects stored on disk. | ||
| Validator(validate bool) (validation.Schema, error) | ||
| Validator(validationDirective string, verifier *resource.QueryParamVerifier) (validation.Schema, error) |
There was a problem hiding this comment.
I missed it in my first review. This interface can't change. It would be backwards compatibility breaking.
|
ok, it seems |
There was a problem hiding this comment.
To document why I'm approving this breaking change.
The Factory interface in Helm is a mirror of the Factory interface in kubectl. Kubernetes makes breaking changes in minor versions. This is ultimately a situation of Helm leaking the Kubernetes breaking change. We have a policy that we can't fix for Kubernetes breaking changes. So, this interface needs to change.
Why not use the kubectl interface directly? As I understand it, we do this to minimize the breaking surface area. For example, we don't need every function on the interface so addition of new ones don't need to cause breaking changes. This is a situation where we need to bubble up the change.
As a follow-up, I'll document this in more detail.
| validationDirective := metav1.FieldValidationIgnore | ||
| if validate { | ||
| validationDirective = metav1.FieldValidationStrict | ||
| } | ||
|
|
||
| dynamicClient, err := c.Factory.DynamicClient() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| verifier := resource.NewQueryParamVerifier(dynamicClient, c.Factory.OpenAPIGetter(), resource.QueryParamFieldValidation) | ||
| schema, err := c.Factory.Validator(validationDirective, verifier) |
There was a problem hiding this comment.
Note to other Helm reviewers, this is roughly how kubectl does it.
@mattfarina I think it would be great to document this before the reasoning behind this is lost. |
This required modifying the
kube.Factoryinterface to conform tochanges in k8s'
cmdutil.Factoryinterface:kubernetes/kubernetes@fe37728
Signed-off-by: Andrew Seigner andrew@sig.gy
What this PR does / why we need it:
Bumps the k8s dependencies from 0.23.6 to 0.24.0.
Special notes for your reviewer:
This dependency bump required modifying source code beyond
go.mod/go.sum.If applicable: