-
Notifications
You must be signed in to change notification settings - Fork 24
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
updating dependencies and code to support k8s 1.28 #51
updating dependencies and code to support k8s 1.28 #51
Conversation
4d917d5
to
e6d0fbb
Compare
Looks like the e2e tests are failing due to
|
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 |
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/[email protected]/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 |
aec9606
to
f252bcb
Compare
internal/ansible/flags/flag.go
Outdated
flagSet.StringVar(&f.LeaderElectionResourceLock, | ||
"leader-elect-resource-lock", | ||
"configmapsleases", | ||
"The type of resource object that is used for locking during leader election."+ | ||
" Supported options are 'leases', 'endpointsleases' and 'configmapsleases'. Default is configmapsleases.", | ||
) |
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 don't think we can just remove this without it being a breaking change and requiring a new major version. I'd prefer if we changed this to something like:
flagSet.StringVar(&f.LeaderElectionResourceLock,
"leader-elect-resource-lock",
resourcelock.LeasesResourceLock,
"determines which resource lock to use for leader election, defaults to \"leases\". Change this value only if you know what you are doing.",
)
and then remove the hardcoded setting below
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 think it's also acceptable to deprecate this flag and have it "do nothing". We just can't remove it right now
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 flag isn't in the helm-plugin
so I was trying to remove it for consistency/clarity across plugins. I'll move to deprecate it, since IMO that's more clear to end users.
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.
Marking as "Request Changes" to properly signal the review status of this PR from my end. Reasoning for requesting changes is https://github.com/operator-framework/ansible-operator-plugins/pull/51/files#r1457477413
f252bcb
to
76fb233
Compare
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.
/lgtm
Will merge pending passing CI
Signed-off-by: Adam D. Cornett <[email protected]>
76fb233
to
f28e319
Compare
New changes are detected. LGTM label has been removed. |
Description of the change:
pip
dependencies and container dependencies so that container can be builtMotivation for the change:
To keep project current.