-
Notifications
You must be signed in to change notification settings - Fork 161
bump controller-runtime to 0.23.0 #6080
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
Conversation
|
I think we should slow down a bit here. This isn't the only thing that this release introduces and we should take a step back and evaluate. This can serve as a kick-off for more broader changes to the structure and layout of the project. If the dependency update is this pressing, I'd prefer if we keep the changes minimal by continuing to use the CustomValidator. |
Release introduces a few breaking changes:
|
| ) | ||
|
|
||
| func missingDatabaseExecutorImage(_ context.Context, _ *Validator, dk *dynakube.DynaKube) string { | ||
| func missingDatabaseExecutorImage(_ context.Context, _ *validatorClient, dk *dynakube.DynaKube) string { |
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.
NB (for reviwer):
we can also rename validator to validatorType or something similar
type validator[T DynaKubeType] struct {
*validatorClient
}
and validatorClient back to Validator to reduce amount of changes in this PR
avorima
left a comment
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.
missing RBAC for events.k8s.io
| ) | ||
|
|
||
| func invalidActiveGateProxyURL(ctx context.Context, dv *Validator, dk *dynakube.DynaKube) string { | ||
| func invalidActiveGateProxyURL(ctx context.Context, dv *validatorClient, dk *dynakube.DynaKube) string { |
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.
rename variable to vc to stay consistent. also in
pkg/api/validation/dynakube/telemetryservice.go
pkg/api/validation/dynakube/validation.go
pkg/api/validation/edgeconnect/validation.go
| ) | ||
|
|
||
| type Validator struct { | ||
| type DynaKubeType interface { |
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.
this was aleady mentioned in a previous discussion, but specialization doesn't really do much at the cost of having to be kept up-to-date. the getDynaKube function already checks the type at runtime and it's very obvious when you use a non-supported type
| type validatorFunc func(ctx context.Context, dv *Validator, dk *dynakube.DynaKube) string | ||
| type updateValidatorFunc func(ctx context.Context, dv *Validator, oldDk *dynakube.DynaKube, newDk *dynakube.DynaKube) string | ||
| type validatorFunc func(ctx context.Context, dv *validatorClient, dk *dynakube.DynaKube) string | ||
| type updateValidatorFunc func(ctx context.Context, dv *validatorClient, oldDk *dynakube.DynaKube, newDk *dynakube.DynaKube) string |
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.
nit
| type updateValidatorFunc func(ctx context.Context, dv *validatorClient, oldDk *dynakube.DynaKube, newDk *dynakube.DynaKube) string | |
| type updateValidatorFunc func(ctx context.Context, dv *validatorClient, oldDk, newDk *dynakube.DynaKube) string |
| if err := v1beta5.SetupWebhookWithManager(mgr, validator); err != nil { | ||
| v1beta5Validator := newGenericValidator[*v1beta5.DynaKube](validatorImpl) | ||
| if err := ctrl.NewWebhookManagedBy(mgr, &v1beta5.DynaKube{}). | ||
| WithValidator(v1beta5Validator). // will create an endpoint at /validate-dynatrace-com-v1beta4-dynakube |
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.
comments don't reference the correct endpoint. i think it's a sign that they can be removed
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.
| ) | ||
|
|
||
| func isModuleDisabled(_ context.Context, v *Validator, _ *edgeconnect.EdgeConnect) string { | ||
| func isModuleDisabled(_ context.Context, v *validatorClient, _ *edgeconnect.EdgeConnect) string { |
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.
vc to stay consistent
|
|
||
| func SetupWebhookWithManager(mgr ctrl.Manager) error { | ||
| validator := New(mgr.GetAPIReader(), mgr.GetConfig()) | ||
| validatorImpl := newClient(mgr.GetAPIReader(), mgr.GetConfig()) |
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.
please align dynakube and edgeconnect validation packages. i prefer the way it's done in edgeconnect since it's more concise
- validatorClient
- newValidator instead of newGenericValidator and just call it inline in WithValidator
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'm aware of it - i'll fix it!
Description
Similar to #6053 but includes changes to fix CI.
Note
controller-runtime has breaking changes more details https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.23.0
DAQ-20001
How can this be tested?
unit-test/ e2e tests where we can check webhooks.