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

add tamilmani1989 and wedaly to new cilium-cni group #182

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tamilmani1989
Copy link
Contributor

@tamilmani1989 tamilmani1989 commented Dec 10, 2024

Would like to create new group for cilium cni and add myself and @wedaly as part of it since we contributed to delegated ipam and continue to participate in reviews and PRs related to this area

Signed-off-by: Tamilmani <tamanoha@microsoft.com>
@joestringer
Copy link
Member

Hi @tamilmani1989 , thanks for helping to maintain Cilium 🎉 . I think this will need a bit more discussion. Some things that would help:

  • It's not clear to me what the intended scope of this group is. It sounds like the focus is delegated IPAM, but the name of the group suggests a wider scope. Perhaps you could post a PR on cilium/cilium with the proposed codeowners changes and we can discuss further from there.
  • It's good practice to have one or more committers in a reviewer group to help steer the group. Good candidates would be people who are already an owner for the files that this new group should own.

@tamilmani1989
Copy link
Contributor Author

tamilmani1989 commented Dec 12, 2024

It's not clear to me what the intended scope of this group is. It sounds like the focus is delegated IPAM, but the name of the group suggests a wider scope. Perhaps you could post a PR on cilium/cilium with the proposed codeowners changes and we can discuss further from there.

since delegated ipam is part of CNI spec, i inclined to create cilium-cni and also the surface is not bigger. I can update if you want to scope down to just delegated-ipam

It's good practice to have one or more committers in a reviewer group to help steer the group. Good candidates would be people who are already an owner for the files that this new group should own.

I'm not sure whom to add that's why didn't add other ppl. Let me check owner of those files

@tamilmani1989
Copy link
Contributor Author

looks like plugin/cilium-cni is owned by @cilium/sig-k8s and sig-k8s owns several other stuff. https://github.com/cilium/cilium/blob/fb5cf48a42e15a3c5cd01aeb880edd7682d59961/CODEOWNERS#L618C22-L618C37

@tommyp1ckles
Copy link
Contributor

I could maybe see an argument for why CNI should have its own group. It's @cilium/sig-k8s adjacent but IMO doesn't exactly neatly fall into that bucket (the CNI spec is not actually part of k8s). As a reviewer, @cilium/sig-k8s can feel broad at times, getting load balanced for reviewing a cilium-cni change would be more of a context switch for me than a typical k8s type change (i.e. watchers, CRD specs, etc...).

cc @squeed

@joestringer
Copy link
Member

Just revisiting this fresh in the new year, to reiterate my understanding of the latest on this topic:

  • I think that it likely makes sense to have a scope something like this per @tommyp1ckles' feedback, however I am still looking for a PR in cilium/cilium to demonstrate the intended codeowner review areas and to summarize the scope according to the way that other groups are organized.
  • We'll want to ensure that there is some continuity in the ownership for these areas, either by including some volunteers from the existing @cilium/sig-k8s group or from other contributors who have a track record of contribution in the area covered by the new codeowner.
  • As with all reviewers in the Cilium project, we also expect that members of the new group have a track record of contribution so we can maintain consistency in reviews over time. As such, links to demonstrate your own contributions in this area would help. Given that you have mentioned the involvement in delegated IPAM this should be straightforward to demonstrate.

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 this pull request may close these issues.

None yet

3 participants