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

labels: Add wg/ipam group label #3455

Merged
merged 1 commit into from
Jun 5, 2024
Merged

labels: Add wg/ipam group label #3455

merged 1 commit into from
Jun 5, 2024

Conversation

oshoval
Copy link
Contributor

@oshoval oshoval commented Jun 5, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:


@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jun 5, 2024
@oshoval
Copy link
Contributor Author

oshoval commented Jun 5, 2024

/cc @enp0s3 @maiqueb @qinqon @brianmcarey

can you please take a look ?
requested by Igor

@oshoval oshoval marked this pull request as draft June 5, 2024 06:35
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2024
@fabiand
Copy link
Member

fabiand commented Jun 5, 2024

/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2024
@fabiand
Copy link
Member

fabiand commented Jun 5, 2024

Folks, I'm really wondering what is going on.

How the heck did we end up with a sig-wg-* prefix?
Who defined the semantics?
Can everybody invent sigs?

PR's like this require us to think about how we formalize - to add more prorcesses that nobody wants - the introductoin of sigs - to define ownership in KubeVirt.

Again: SIGs to define ownership of non-overlapping areas within KubeVirt.
A SIG can then decide on itself how to divide it's area among members. One idea is to use wg-*.

@fabiand
Copy link
Member

fabiand commented Jun 5, 2024

/cc @vladikr fyi - how things start to move if we do not solidify the model aroudn sigs etc.

@oshoval
Copy link
Contributor Author

oshoval commented Jun 5, 2024

Np thanks, it was draft and i raised a question on slack to understand what name to use
Do you mean sig-network-wg-ipam ? or just wg-ipam ?

@fabiand
Copy link
Member

fabiand commented Jun 5, 2024

Maybe I was a little bit harsh, I see now it's sig/wg-ipam, thus not sig-wg-ipam.

We can also not just define wg-ipam - it must be associated to a SIG. How do we do this?

Once it is solved, then please use wg/ipam instead of sig/wg-ipam.

@fabiand
Copy link
Member

fabiand commented Jun 5, 2024

It feels like sig/network plus a new label wg/ipam could indicate the right thing:

Owned by sig network, handled by wg ipam.

@oshoval
Copy link
Contributor Author

oshoval commented Jun 5, 2024

hmm, not sure i did correct (pushed change before i read newest comments)
will need to understand better how to do what you suggested
atm changed to sig/network-wg-ipam

@oshoval
Copy link
Contributor Author

oshoval commented Jun 5, 2024

updated the label to wg/ipam
hope i understood you correct (once i do, i will update the owners file in the other PR with all the required changes)
thanks

@oshoval oshoval changed the title labels: Add sig-wg-ipam group label labels: Add wg/ipam group label Jun 5, 2024
@enp0s3
Copy link
Contributor

enp0s3 commented Jun 5, 2024

Folks, I'm really wondering what is going on.

How the heck did we end up with a sig-wg-* prefix? Who defined the semantics? Can everybody invent sigs?

PR's like this require us to think about how we formalize - to add more prorcesses that nobody wants - the introductoin of sigs - to define ownership in KubeVirt.

Again: SIGs to define ownership of non-overlapping areas within KubeVirt. A SIG can then decide on itself how to divide it's area among members. One idea is to use wg-*.

@fabiand @vladikr This PR just reflects an attempt to comply with what was discussed on Monday's approvres meeting.
IIUC there was a push back from having individual approvers in specific packages, thus the idea with WG came up.

Now to be more specific, with regards to IPAM PR
The situation is simple - ex sig-network maintainers are willing to maintain IPAM package, while current sig-network maintainers don't.

If the WG procedure is yet defined, can we proceed with having individual approvers in pkg/libipam/OWNERS?

docs/labels.md Outdated
@@ -84,6 +84,7 @@ larger set of contributors to apply/remove them.
| <a id="triage/needs-information" href="#triage/needs-information">`triage/needs-information`</a> | Indicates an issue needs more information in order to work on it.| anyone | |
| <a id="triage/not-reproducible" href="#triage/not-reproducible">`triage/not-reproducible`</a> | Indicates an issue can not be reproduced as described.| anyone | |
| <a id="triage/unresolved" href="#triage/unresolved">`triage/unresolved`</a> | Indicates an issue that can not or will not be resolved.| anyone | |
| <a id="wg/ipam" href="#wg/ipam">`wg/ipam`</a> | | anyone | [label](https://prow.ci.kubevirt.io/command-help#label) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this comment, please sync to sig-network charter in community repo. In sig network charter, please add wg-ipam.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated this PR (hope i understood correct)
and created kubevirt/community#300

@fabiand
Copy link
Member

fabiand commented Jun 5, 2024

@enp0s3 understood.

My main concern was abut the usage of the string sig-wg-iapm (which was a misunderstanding on my side).

As stated above:
sigs own code.
wg are internal structures to delegate who is handling it.

thus we
a) need to make the connection between wg ipam and sig network
b) sig network remains to own the code/area, wg ipam will handle that specific piece and has trust by sig network to even approve it.

To me the current PR is thus much better, as it's introducing wg/ipam.

I'd expect the owners file to then include buth

approvers
- wg/ipam
 - sig/network

unless the file is anyway in a sig-network owned directory, then the sig-network can be omitted because it's getting inherited.

oshoval added a commit to oshoval/kubevirt that referenced this pull request Jun 5, 2024
WG IPAM is part of sig-network and will maintain
the IPAM for secondary networks feature [1].

[1] kubevirt#11410

Charter: kubevirt/community#300
Label: kubevirt/project-infra#3455

Signed-off-by: Or Shoval <[email protected]>
@fabiand
Copy link
Member

fabiand commented Jun 5, 2024

@oshoval thanks for your openess! and the quick turnaround

The PR including the IMPORTANT links (for user context) to community and label PRs are good.

/unhold

/lgtm

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2024
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2024
@oshoval oshoval marked this pull request as ready for review June 5, 2024 08:49
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2024
@oshoval
Copy link
Contributor Author

oshoval commented Jun 5, 2024

Thanks Fabian for the help here

@brianmcarey @enp0s3 can you please take a look ?

@dhiller
Copy link
Contributor

dhiller commented Jun 5, 2024

All, sorry for not being able to bring the PR around the initial wgs for arch to life soon enough: kubevirt/community#298

/approve

Thanks @oshoval

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhiller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2024
@kubevirt-bot kubevirt-bot merged commit 5fae7e4 into kubevirt:main Jun 5, 2024
6 checks passed
@kubevirt-bot
Copy link
Contributor

@oshoval: Updated the label-config configmap in namespace kubevirt-prow at cluster default using the following files:

  • key labels.yaml using file github/ci/prow-deploy/kustom/base/configs/current/labels/labels.yaml

In response to this:

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

oshoval added a commit to oshoval/kubevirt that referenced this pull request Jun 5, 2024
WG IPAM will maintain
the IPAM for secondary networks feature [1].

[1] kubevirt#11410

Charter: kubevirt/community#300
Label: kubevirt/project-infra#3455

Signed-off-by: Or Shoval <[email protected]>
oshoval added a commit to oshoval/project-infra that referenced this pull request Jun 6, 2024
kubevirt-bot pushed a commit that referenced this pull request Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants