Codecov Report

Merging#442 (2d9198f) intomaster (3b690b2) willdecrease coverage by0.05%.
The diff coverage is37.50%.

❗ Current head2d9198f differs from pull request most recent head95b3337. Consider uploading reports for the commit95b3337 to get more accurate results

@@            Coverage Diff             @@##           master     #442      +/-   ##==========================================- Coverage   49.33%   49.27%   -0.06%==========================================  Files          56       56                Lines        4966     4980      +14     ==========================================+ Hits         2450     2454       +4- Misses       2265     2275      +10  Partials      251      251

Flags with carried forward coverage won't be shown.Click here to find out more.

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update3b690b2...95b3337. Read thecomment docs.

HI@grzesuav and@AmitKumarDas, could you please review when you have free time? I left this in draft because I have TODOs around naming and will update after feedback.

hi @mjsmith1028 , it would be good to have consistency withhttps://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/leaderelection/leader_election.go.

About cmd line options, I would make them the same as config options (i.e.leaderElectionEnabled, etc). I didn't went trough leader election in ctrl-runtime so cannot give more precise advice at the moment. I am aware that multiple resources can be used as leader key object, please make sure to use newest ones (I remember that ctrl-runtime is going away from configmaps to another resources)

Thanks for PR!

@mjsmith1028 Did you run some tests to verify if nothing breaks. Is it feasible for you to add some tests / examples that can verify this behaviour?
@grzesuav Is metacontroller now making use of controller-runtime such that just exposing the latter's arguments enable metacontroller to achieve what controller-runtime is capable of?

hi @mjsmith1028 , it would be good to have consistency withhttps://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/leaderelection/leader_election.go.

About cmd line options, I would make them the same as config options (i.e.leaderElectionEnabled, etc). I didn't went trough leader election in ctrl-runtime so cannot give more precise advice at the moment. I am aware that multiple resources can be used as leader key object, please make sure to use newest ones (I remember that ctrl-runtime is going away from configmaps to another resources)

Thanks for PR!

@grzesuav Thanks! I updated to expose all the leader election options and defaulted to the preferred resource lock leases. I'll add some documentation around this. I am wondering if there is any concern about the growing number of arguments passed to metacontroller or if you are fine with that approach?

@mjsmith1028 Did you run some tests to verify if nothing breaks. Is it feasible for you to add some tests / examples that can verify this behaviour?@grzesuav Is metacontroller now making use of controller-runtime such that just exposing the latter's arguments enable metacontroller to achieve what controller-runtime is capable of?

@AmitKumarDas I only ran some small manual testing around this to verify a leader was elected and the other replica waits until the leader is in a bad state before becoming the new leader. I can definitely add an example. I'm not 100% sure on the approach I should take for testing but I'll think on it. Do you have any ideas?

Hi@AmitKumarDas and@grzesuav, I cleaned up this PR and it is ready to review. Please let me know if you have anymore feedback.

FYI I rebased to clean up the commit history.

🎉 This PR is included in version 2.3.0 🎉

The release is available onGitHub release

Yoursemantic-release bot 📦🚀