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

Add Jeffwan to OWNERS files #1443

Merged
merged 2 commits into from
Aug 5, 2020
Merged

Add Jeffwan to OWNERS files #1443

merged 2 commits into from
Aug 5, 2020

Conversation

Jeffwan
Copy link
Member

@Jeffwan Jeffwan commented Jul 31, 2020 •
edited
Loading

Which issue is resolved by this Pull Request:

Description of your changes:
Here's the contributions I made. https://github.com/kubeflow/manifests/pulls?page=2&q=is%3Apr+author%3AJeffwan+is%3Aclosed Besides AWS changes, I make lots of contributions on common components like training operators, etc. I would like to become an approver to better help community members.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@Jeffwan Jeffwan requested a review from jlewi July 31, 2020 18:37
@kubeflow-bot
Copy link
Contributor

This change is Reviewable

@google-cla google-cla bot added the cla: yes label Jul 31, 2020
@PatrickXYS
Copy link
Member

@Jeffwan has made great improvements on kubeflow/manifests!

@jlewi
Copy link
Contributor

jlewi commented Aug 3, 2020

Per: https://www.kubeflow.org/docs/about/contributing/#maintaining-owners-files

Do we need to keep growing the number of approvers in the root owners file as opposed to growing people in sub owners files?

@Jeffwan
Copy link
Member Author

Jeffwan commented Aug 3, 2020

@jlewi

OWNERS are kind of outdated. We didn't do clean up and I think that's the reason we think it keep growing. I touch different components and probably I can add OWNER files in different sub folders. In this way, I can help review PR but may not be able to help approve PR in other sub folders.

@jlewi
Copy link
Contributor

jlewi commented Aug 4, 2020

@Jeffwan You are right root OWNERs file is outdated. If you want to remove inactive approvers and add yourself so that the overall # of root approvers isn't growing that makes sense to me.

We should also have OWNERs files in all of the top level directories and should focus on growing those.

Can you create owners files for any top level directories for which you are one of the primary owners and is currently missing an owners file?

@Jeffwan
Copy link
Member Author

Jeffwan commented Aug 4, 2020 •
edited
Loading

@jlewi

Agree. Let me make two changes and you can help review.

  1. Create OWNER files in top level directories and add myself.
  2. Clean up some approvals not active in last 3 or 6 months.

@Jeffwan Jeffwan mentioned this pull request Aug 4, 2020
Merged
1 task
@Jeffwan Jeffwan force-pushed the add_shjiaxin_owner branch from d1160ea to 22e3637 Compare August 4, 2020 16:34
@Jeffwan Jeffwan force-pushed the add_shjiaxin_owner branch from 22e3637 to 0a61fac Compare August 4, 2020 16:35
@Jeffwan Jeffwan changed the title Add Jeffwan as approver Add Jeffwan to OWNERS files Aug 4, 2020
@Jeffwan
Copy link
Member Author

Jeffwan commented Aug 4, 2020 •
edited
Loading

@jlewi

I create a separate PR to remove approvers without PR/issues in last 6 months. I update this PR to add myself to some of the folders I am actively working on

jlewi
jlewi reviewed Aug 4, 2020
View reviewed changes
tf-training/OWNERS
@@ -1,4 +1,5 @@
approvers:
- andreyvelich
- gaocegege
Copy link
Contributor

Choose a reason for hiding this comment

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

@andreyvelich @gaocegege @johnugeorge Can one of you LGTM this since you are already in the OWNERs file?

jlewi
jlewi reviewed Aug 4, 2020
View reviewed changes
tensorboard/OWNERS Outdated
@@ -0,0 +1,2 @@
approvers:
- Jeffwan
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just delete this directory per:
#415

Copy link
Member Author

Choose a reason for hiding this comment

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

Some Kfdefs has dependency on this. Let me file a clean up PR. Let's merge the work to kubeflow/kubeflow#3578

Copy link
Member Author

Choose a reason for hiding this comment

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

#1459 @jlewi Address #415 and I remove this folder

jlewi
jlewi reviewed Aug 4, 2020
View reviewed changes
pytorch-job/OWNERS
@@ -1,4 +1,5 @@
approvers:
- andreyvelich
Copy link
Contributor

Choose a reason for hiding this comment

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

@andreyvelich @gaocegege @johnugeorge please LGTM

Copy link
Member

Choose a reason for hiding this comment

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

/LGTM

Thanks for your contribution! 🎉 👍

jlewi
jlewi reviewed Aug 4, 2020
View reviewed changes
jupyter/OWNERS
@@ -1,4 +1,5 @@
approvers:
- Jeffwan
Copy link
Contributor

Choose a reason for hiding this comment

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

@kimwnasptd please LGTM

@Jeffwan Suggest removing @lluunn

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

andreyvelich
andreyvelich reviewed Aug 4, 2020
View reviewed changes
Copy link
Member

@andreyvelich andreyvelich 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 for your contributions @Jeffwan!
/lgtm

spark/OWNERS Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 4, 2020
@kimwnasptd
Copy link
Member

/lgtm

@jlewi
Copy link
Contributor

jlewi commented Aug 5, 2020

/lgtm
/approve

Thanks @Jeffwan

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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 b3f8a28 into kubeflow:master Aug 5, 2020
@Jeffwan Jeffwan mentioned this pull request Aug 5, 2020
Merged
k8s-ci-robot pushed a commit that referenced this pull request Aug 7, 2020
@Jeffwan
Automated cherry pick of #1443: Add Jeffwan to OWNERS files (#1464)
7565f69 
* Add Jeffwan to OWNERS files

* Address code review feedbacks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreyvelich andreyvelich andreyvelich left review comments

@gaocegege gaocegege gaocegege left review comments

@jlewi jlewi jlewi left review comments

@krishnadurai krishnadurai Awaiting requested review from krishnadurai

@PatrickXYS PatrickXYS Awaiting requested review from PatrickXYS

Assignees

@jlewi jlewi

@gaocegege gaocegege

@kimwnasptd kimwnasptd

@andreyvelich andreyvelich

Labels
approved cla: yes lgtm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Jeffwan @kubeflow-bot @PatrickXYS @jlewi @kimwnasptd @k8s-ci-robot @gaocegege @andreyvelich