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

OWNERS: enable SIGs, WGs and automation code writers to own their folders #360

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dhiller
Copy link
Contributor

@dhiller dhiller commented Nov 28, 2024

What this PR does / why we need it:

Copies the structure of k/kubevirt OWNERS_ALIASES, adjusts the current root OWNERS to it (by moving the OWNERS approvers etc. into root approvers, reviewers and emeritus).

Then adds OWNERS files referring to the alias entries inside the sig and wg directories.

Finally creates groups for owning the automation code (code-reviewers vs code approvers) to enable separating reviews from the work root approvers have to do.

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:

/cc @aburdenthehand @vladikr @fabiand

FYI @EdDev @alicefr @rthallisey @vamsikrishna-siddu @iholder101 @0xFelix @lyarwood

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Nov 28, 2024
@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vladikr for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

Looks good to me I'd say

@@ -0,0 +1,3 @@
# TODO: do not forget to add the chairs as approvers and other stakeholders as reviewers
approvers: []
reviewers: []
Copy link
Member

Choose a reason for hiding this comment

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

nit: no newline at EOF

Copy link
Member

Choose a reason for hiding this comment

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

@dhiller Looks like this pesky wabbit persists

@dhiller dhiller changed the title OWNERS: add OWNERS and OWNERS_ALIASES OWNERS: enable SIGs, WGs and automation code writers to own their charter folders Nov 29, 2024
@dhiller dhiller changed the title OWNERS: enable SIGs, WGs and automation code writers to own their charter folders OWNERS: enable SIGs, WGs and automation code writers to own their folders Nov 29, 2024
Copy link
Contributor

@iholder101 iholder101 left a comment

Choose a reason for hiding this comment

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

Thank you @dhiller! Looking good!
Left some questions below

OWNERS_ALIASES Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

How should we sync between changes here and in k/k?
IOW, if someone is added as an approver for a SIG, would he need to also add himself to here in a different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no automation in place to sync this currently - the only automation that achieves a (though stripped down) sync is the one that keeps the OWNERS file in k/project-infra in sync with the source repositories.

I haven't yet found the time to create an issue on that - I'll do that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +1 to +2
approvers:
- code-approvers
Copy link
Contributor

Choose a reason for hiding this comment

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

How are code-approvers different than approvers, same for code-reviewers and reviewers? why do we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code-approvers is the group of people that are maintaining the automation code in this repository. Since I couldn't imagine a better name, I stuck to what I found in k/kubevirt, from which this comes - if you insist I could rename it to i.e. automation-code-reviewers and automation-code-approvers or do you have a better suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer automation-code-* as it is more descriptive.
But I'm also perfectly content to not change it since this matches k/kubevirt and the entry has a descriptive note.

@dhiller
Copy link
Contributor Author

dhiller commented Dec 17, 2024

It would be great if we could get this in - I am currently blocked on a couple of PRs wrt community automation that require reviews...

FYI @aburdenthehand

@@ -0,0 +1,3 @@
# TODO: do not forget to add the chairs as approvers and other stakeholders as reviewers
approvers: []
reviewers: []
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this is the same issue Felix mentioned: no newline at EOF

Copy link
Member

@aburdenthehand aburdenthehand left a comment

Choose a reason for hiding this comment

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

Great stuff @dhiller. There's a nit here that I've added to; aside from that I'm happy to get this in.

@iholder101
Copy link
Contributor

Thank you @dhiller!
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2024
Copies the structure of k/kubevirt OWNERS_ALIASES, adjusts the current
root OWNERS to it - specifically by moving the OWNERS approvers etc.
into root approvers, reviewers and emeritus.

Then adds OWNERS files referring to the alias entries inside the sig
and wg directories.

Finally this creates groups for owning the automation code
code-reviewers vs code approvers to enable separating reviews from the
work root approvers have to do.

Signed-off-by: Daniel Hiller <[email protected]>
The OWNERS pluging doesn't like referring untrusted people inside
OWNERS_ALIASES so we move them towards regular OWNERS emeritus_approvers
section.

Signed-off-by: Daniel Hiller <[email protected]>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2024
@dhiller
Copy link
Contributor Author

dhiller commented Dec 18, 2024

@aburdenthehand @iholder101 thanks for your reviews, fixed the missing newline.

PTAL 🙏

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2024
@kubevirt-bot
Copy link

Pull requests that are marked with lgtm should receive a review
from an approver within 1 week.

After that period the bot marks them with the label needs-approver-review.

/label needs-approver-review

@kubevirt-bot kubevirt-bot added the needs-approver-review Indicates that a PR requires a review from an approver. label Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. needs-approver-review Indicates that a PR requires a review from an approver. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants