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.

Sign up for GitHub

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

Jump to bottom

Owners: Promote Gacko to ingress-nginx-maintainers & ingress-nginx-reviewers. #11165

Merged
merged 6 commits into from
Apr 4, 2024
Merged

Owners: Promote Gacko to ingress-nginx-maintainers & ingress-nginx-reviewers. #11165

merged 6 commits into from
Apr 4, 2024

Conversation

Gacko
Copy link
Member

@Gacko Gacko commented Mar 26, 2024

What this PR does / why we need it:

This PR promotes myself to ingress-nginx-maintainers & ingress-nginx-reviewers.

It also re-organizes the existing owners and owner aliases to remove duplicates in aliases and inheritance.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • CVE Report (Scanner found CVE and adding report)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Which issue/s this PR fixes

https://kubernetes.slack.com/archives/C021E147ZA4/p1711446319357989

How Has This Been Tested?

Not at all. CI will show us.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added unit and/or e2e tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2024
@netlify Netlify
Copy link

netlify bot commented Mar 26, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-ingress-nginx ready!

Name Link
🔨 Latest commit db604e9
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/6602bc45474abf00086e3039
😎 Deploy Preview https://deploy-preview-11165--kubernetes-ingress-nginx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot requested review from puerco and strongjz March 26, 2024 11:14
@k8s-ci-robot k8s-ci-robot added the area/lua Issues or PRs related to lua code label Mar 26, 2024
@Gacko
Copy link
Member Author

Gacko commented Mar 26, 2024

/triage accepted
/kind cleanup
/priority backlog
/cherry-pick release-1.10

@k8s-infra-cherrypick-robot
Copy link
Contributor

@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you.

In response to this:

/triage accepted
/kind cleanup
/priority backlog
/cherry-pick release-1.10

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority labels Mar 26, 2024
@Gacko
Copy link
Member Author

Gacko commented Mar 26, 2024

/cc @rikatz @tao12345666333 @cpanato

cpanato
cpanato requested changes Mar 26, 2024
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

some comments

OWNERS_ALIASES Show resolved Hide resolved
OWNERS_ALIASES Show resolved Hide resolved
OWNERS_ALIASES Show resolved Hide resolved
images/kube-webhook-certgen/OWNERS Show resolved Hide resolved
OWNERS_ALIASES Show resolved Hide resolved
@tao12345666333
Copy link
Member

Thank you for your contributions. I'm happy to see this.
However, there are some errors in the reorganization process. Please be aware not to remove any other permissions.

@Gacko
Copy link
Member Author

Gacko commented Mar 26, 2024

According to my understanding from the docs, owners are getting inherited. At least that already was the case for the chart. You, @tao12345666333, got requested for review even though you're not on the ingress-nginx-helm-reviewers but on ingress-nginx-reviewers.

I therefore assumed being on the alias with the most privileges only should be enough.

@Gacko
Copy link
Member Author

Gacko commented Mar 26, 2024

@cpanato @tao12345666333 Some details about the inheritance, I emphasized the important part: https://github.com/kubernetes/community/blob/master/contributors/guide/owners.md#owners

OWNERS

Each directory that contains a unit of independent code or content may also contain an OWNERS file.
This file applies to everything within the directory, including the OWNERS file itself, sibling
files, and child directories.

OWNERS files are in YAML format and support the following keys:

  • approvers: a list of GitHub usernames or aliases that can /approve a PR
  • labels: a list of GitHub labels to automatically apply to a PR
  • options: a map of options for how to interpret this OWNERS file, currently only one:
    • no_parent_owners: defaults to false if not present; if true, exclude parent OWNERS files.
      Allows the use case where a/deep/nested/OWNERS file prevents a/OWNERS file from having any
      effect on a/deep/nested/bit/of/code
  • reviewers: a list of GitHub usernames or aliases that are good candidates to /lgtm a PR
  • emeritus_approvers a list of GitHub usernames of folks who were previously in the approvers section,
    but are no longer actively approving code. please see below for more details.

The above keys constitute a simple OWNERS configuration.

All users are expected to be assignable. In GitHub terms, this means they must be
members of the organization to which the repo belongs.

@Gacko Gacko requested a review from cpanato March 26, 2024 11:59
@Gacko
Copy link
Member Author

Gacko commented Mar 26, 2024

One example of @rikatz approving a chart PR even though he's not a member of ingress-nginx-helm-maintainers: #11109

@Gacko
Copy link
Member Author

Gacko commented Mar 26, 2024 •
edited
Loading

And another one @tao12345666333 approved even though he's not a member of the ingress-nginx-helm-maintainers nor a repository admin: #10409

Gacko added 5 commits March 26, 2024 13:14
@Gacko
Owners: Update URL in aliases.
be95996
@Gacko
Images: Remove owners as it's identical to global owners.
9648912
@Gacko
Images: Remove global owners from kube-webhook-certgen owners.
18da675
@Gacko
Owners: Remove members from aliases covered by other aliases.
3b43dbf 
ingress-nginx-helm-maintainers:
- cpanato: Covered by ingress-nginx-maintainers
- strongjz: Covered by ingress-nginx-maintainers

ingress-nginx-helm-reviewers:
- cpanato: Covered by ingress-nginx-reviewers
- strongjz: Covered by ingress-nginx-reviewers

ingress-nginx-docs-maintainers:
- tao12345666333: Covered by ingress-nginx-maintainers
@Gacko
Owners: Promote myself to ingress-nginx-maintainers & `ingress-ngin…
db604e9 
…x-reviewers`.
@k8s-ci-robot k8s-ci-robot added the area/helm Issues or PRs related to helm charts label Mar 26, 2024
@msfidelis
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

@msfidelis: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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/test-infra repository.

@Gacko
Copy link
Member Author

Gacko commented Mar 27, 2024

/honk

@cpanato

@k8s-ci-robot
Copy link
Contributor

@Gacko:
goose image

In response to this:

/honk

@cpanato

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/test-infra repository.

@cpanato cpanato added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 29, 2024
cpanato
cpanato reviewed Mar 29, 2024
View reviewed changes
images/kube-webhook-certgen/OWNERS

approvers:
- ingress-nginx-admins
- ingress-nginx-maintainers
Copy link
Member

Choose a reason for hiding this comment

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

i think we should keep at least the maintainers or drop the entire file

Copy link
Member Author

Choose a reason for hiding this comment

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

But then we would also revoke reviewer privileges for @invidian. Is this intended?

Copy link
Member

Choose a reason for hiding this comment

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

if the ingress-nginx-maintainers still works here then it is fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should. I searched some older, already approved and merged PRs to check if the inheritance from the root OWNERS works, see above. According to my investigations and the docs it should work!

cpanato
cpanato approved these changes Apr 1, 2024
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold for last review from @rikatz and/or @strongjz

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2024
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 1, 2024
@strongjz
Copy link
Member

strongjz commented Apr 4, 2024

/unhold
/lgtm

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, Gacko, msfidelis, strongjz

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

@k8s-ci-robot k8s-ci-robot merged commit bf3fa53 into kubernetes:main Apr 4, 2024
41 checks passed
@k8s-infra-cherrypick-robot
Copy link
Contributor

@Gacko: new pull request created: #11203

In response to this:

/triage accepted
/kind cleanup
/priority backlog
/cherry-pick release-1.10

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/test-infra repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Apr 4, 2024
Merged
@Gacko Gacko deleted the yhrzv branch April 4, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpanato cpanato cpanato approved these changes

@puerco puerco Awaiting requested review from puerco

@strongjz strongjz Awaiting requested review from strongjz

@rikatz rikatz Awaiting requested review from rikatz

@tao12345666333 tao12345666333 Awaiting requested review from tao12345666333

Assignees

@strongjz strongjz

@cpanato cpanato

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/docs area/helm Issues or PRs related to helm charts area/lua Issues or PRs related to lua code cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Gacko @k8s-infra-cherrypick-robot @tao12345666333 @msfidelis @k8s-ci-robot @strongjz @cpanato