search

operator-framework / ansible-operator-plugins

Experimental extraction/refactoring of the Operator SDK's ansible operator plugin
Apache License 2.0
7 stars 17 forks source link

updating dependencies and code to support k8s 1.28 #51

Closed acornett21 closed 5 months ago

acornett21 commented 5 months ago

Description of the change:

Motivation for the change: To keep project current.

everettraven commented 5 months ago

Looks like the e2e tests are failing due to https://github.com/operator-framework/ansible-operator-plugins/blob/ba5d25a75275830407abaa2570a2fb20285899f6/internal/ansible/flags/flag.go#L165 - need to update this to "leases" I think.

acornett21 commented 5 months ago

Looks like the e2e tests are failing due to

https://github.com/operator-framework/ansible-operator-plugins/blob/ba5d25a75275830407abaa2570a2fb20285899f6/internal/ansible/flags/flag.go#L165

  • need to update this to "leases" I think.

I'm not sure what we want to do for this flag, since there can only be one value now and it's hardcoded to resourcelock.LeasesResourceLock (I have yet to push that update). The helm plugin does not expose this flag, so I'm not sure if removal or deprecation is the best option for this flag.

everettraven commented 5 months ago

Looks like the e2e tests are failing due to https://github.com/operator-framework/ansible-operator-plugins/blob/ba5d25a75275830407abaa2570a2fb20285899f6/internal/ansible/flags/flag.go#L165

  • need to update this to "leases" I think.

I'm not sure what we want to do for this flag, since there can only be one value now and it's hardcoded to resourcelock.LeasesResourceLock (I have yet to push that update). The helm plugin does not expose this flag, so I'm not sure if removal or deprecation is the best option for this flag.

Based on https://github.com/kubernetes-sigs/controller-runtime/blob/6747c42ce33966b0e77cac278c6b9087cc2f51c7/pkg/manager/manager.go#L146-L177 it looks like we should update the default value of that flag to "leases"

acornett21 commented 5 months ago

Based on https://github.com/kubernetes-sigs/controller-runtime/blob/6747c42ce33966b0e77cac278c6b9087cc2f51c7/pkg/manager/manager.go#L146-L177 it looks like we should update the default value of that flag to "leases"

Just to restate again, there can only be one value and it's hardcoded and cannot come from the flag, so IMO the flag doesn't serve a purpose and should either be deprecated or removed. I'd prefer removal since other plugins do not have this flag.

everettraven commented 5 months ago

Based on https://github.com/kubernetes-sigs/controller-runtime/blob/6747c42ce33966b0e77cac278c6b9087cc2f51c7/pkg/manager/manager.go#L146-L177 it looks like we should update the default value of that flag to "leases"

Just to restate again, there can only be one value and it's hardcoded and cannot come from the flag, so IMO the flag doesn't serve a purpose and should either be deprecated or removed. I'd prefer removal since other plugins do not have this flag.

Maybe I'm misunderstanding, but I don't see a reason for this to be hardcoded? This still appears to be a configurable option based on https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.16.3/pkg/manager#Options. The default should be updated to reflect the default used by controller-runtime and the description updated to reflect what controller-runtime recommends, which is that this option only be modified if you know what you are doing.

We most certainly can deprecate this flag, but we unfortunately cannot just remove it.

EDIT: To add onto this, I do see that in client-go the only possible resource lock is "leases" https://github.com/kubernetes/client-go/blob/94205f8c40c617ffa7b5cf1927448cfd2e99ebf5/tools/leaderelection/resourcelock/interface.go#L166-L189

That being said, I think it makes sense to keep this a configurable flag since it is configurable in controller-runtime. Doesn't mean anything but "leases" has to work (and we can document this if necessary) but keeping it around prevents additional work beyond modifying the default if in the future there are new resource lock types added/removed/changed

openshift-ci[bot] commented 5 months ago

New changes are detected. LGTM label has been removed.