Skip to content
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

Nominate Ben Leggett for istio-cni maintainer #1314

Closed
linsun opened this issue Jan 29, 2024 · 19 comments · Fixed by #1323
Closed

Nominate Ben Leggett for istio-cni maintainer #1314

linsun opened this issue Jan 29, 2024 · 19 comments · Fixed by #1323

Comments

@linsun
Copy link
Member

linsun commented Jan 29, 2024

Istio-cni is an important component for sidecar and even a critical component for ambient. I propose to add a few maintainers for the istio-cni node agent and I want to nominate @bleggett due to his outstanding contribution: istio/istio#48253 along with many other contributions and reviews in this component. Opening this issue to discuss this openly. @bleggett, feel free to add your contributions for recording purpose.

@linsun
Copy link
Member Author

linsun commented Jan 29, 2024

cc @istio/wg-networking-maintainers for comment

@howardjohn
Copy link
Member

FWIW I don't think we formally have an "istio-cni maintainer". It is owned by the networking group more broadly. That being said I think Ben probably meets the bar (https://github.com/istio/community/blob/master/ROLES.md#requirements-3) more broadly, or is at least extremely close to doing so ( didn't look very closely here)

@linsun
Copy link
Member Author

linsun commented Jan 29, 2024

Thanks John for your input. Agree we don't have istio-cni maintainer formerly. Depends on folks' input and comfortable level, we can decide to create a dedicated group for istio-cni or have Ben join the networking maintainer group.

tagging a few networking leads for input @ramaraochavali @stevenctl @lizan

@nrjpoddar
Copy link
Member

I think adding a CNI maintainer is a good thing to have specially w/Ambient beta on the horizon. I'm in favor of it and adding @bleggett given other TOC members are good with it.

@linsun
Copy link
Member Author

linsun commented Jan 30, 2024

Thanks Neeraj. Just checked, Ben has 32 merged PR which includes substantial PR(most recent one is the huge in-pod istio-cni PR) as well. I'll let the other networking leads weigh in, if they are okay with adding Ben to networking maintainer group, we can add him there instead of forming the istio-cni maintainer.

@hzxuzhonghu
Copy link
Member

istio-cni is a little different, with more undelying networking expertise requirements. I think a sub-group is necessary.

@zirain
Copy link
Member

zirain commented Jan 30, 2024

istio-cni is a little different, with more undelying networking expertise requirements. I think a sub-group is necessary.

like @istio/wg-networking-maintainers-pilot ?

@ramaraochavali
Copy link
Contributor

Looking at PRs, most of them are in cni area. So it makes sense to create istio-cni and add him

@linsun
Copy link
Member Author

linsun commented Jan 30, 2024

@ramaraochavali @hzxuzhonghu @zirain thank you for the feedback! Seems you all are good with moving forward to promote Ben as istio-cni maintainer.

@stevenctl @lizan any feedback from you, please let me know by end of today.

@zirain
Copy link
Member

zirain commented Jan 30, 2024

To be clear, it's good to create an sub-group named istio/wg-networking-maintainers-cni, and grant authority(both istio/wg-networking-maintainers-cni and istio/wg-networking-maintainers) with /cni folder.

same to #1315 and #1316

@linsun
Copy link
Member Author

linsun commented Jan 30, 2024

I'd like to do a vote from networking maintainers and leads. Leads' vote count, maintainers vote are helpful too

option 1: create dedicated group for istio-cni and promote Ben to istio-cni maintainer
option 2: promote Ben to wg-networking-maintainers.

Please use emojis (option 1 heart, option 2: rocket)

@linsun
Copy link
Member Author

linsun commented Jan 30, 2024

cc @istio/wg-networking-maintainers @howardjohn @stevenctl @lizan @ramaraochavali ^^^^

@linsun
Copy link
Member Author

linsun commented Jan 30, 2024

To be clear, it's good to create an sub-group named istio/wg-networking-maintainers-cni, and grant authority(both istio/wg-networking-maintainers-cni and istio/wg-networking-maintainers) with /cni folder.

same to #1315 and #1316

Let us separate #1315 and #1316 into their own github discussions as nomination/voting is personal.

@howardjohn
Copy link
Member

Subgroups have maintainer access to the parent group too.. so if the goal is to block someones access to "networking", subgroup would not work.

IMO fewer groups is easier to maintain and grok. A networking maintainer doesn't need to be an expert on all things "networking" in Istio -- and I don't any current maintainers are given the breadth of Istio. Part of the responsibilities of a maintainer is not just to approve PRs, but also know when not to approve PRs. For example, if a Maintainer was a CNI pro but had no other expertise (which is not the case for Ben, IMO, just an example), I would simply expect them to not approve PRs outside of CNI.

Note the "pilot" subgroup is a bit different scenario. That was about making a higher bar for a critical code portion. This CNI group discussion is more like a side-group, not sub-group. Generally our groups are all bucketed under WGs, though, which makes the not possible in GitHub. As noted above though, I don't think we need to anyways.

@stevenctl
Copy link
Contributor

+1 to John's comment.
Also big +1 to making @bleggett a maintainer

@linsun
Copy link
Member Author

linsun commented Jan 30, 2024

@stevenctl thanks, can you vote on here with your desired emoji? #1314 (comment)

@linsun
Copy link
Member Author

linsun commented Jan 30, 2024

Added this to the agenda of tmr's WG meeting, please vote here - #1314 (comment)

@srampal
Copy link
Contributor

srampal commented Jan 31, 2024

+1 for @bleggett as maintainer!

@linsun
Copy link
Member Author

linsun commented Jan 31, 2024

Notes from today's WG meeting: @bleggett is approved as networking WG maintainers, there is no objection recorded in the meeting. I'll submit a PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants