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 override plugin to kubernetes/utils #10909

Merged

Conversation

dims
Copy link
Member

@dims dims commented Jan 23, 2019

k8s.io/utils has a large share of problems with CLA, let's please add the override plugin

Change-Id: Ie9c43a13911493b6a2fc5c1d0934dfef265df582

Change-Id: Ie9c43a13911493b6a2fc5c1d0934dfef265df582
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 23, 2019
@k8s-ci-robot k8s-ci-robot added area/prow Issues or PRs related to prow sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 23, 2019
@dims
Copy link
Member Author

dims commented Jan 23, 2019

/assign @cblecker

@dims
Copy link
Member Author

dims commented Jan 23, 2019

/assign @cjwagner @fejta

@spiffxp
Copy link
Member

spiffxp commented Jan 23, 2019

This repo has an admins team, so more than just github admins would get /override privileges: https://github.com/orgs/kubernetes/teams/utils-admins/members

I'm wary of turning this on and forgetting about it and wish we had more clarity on which commits specifically were tripping up the CLA check

@spiffxp
Copy link
Member

spiffxp commented Jan 23, 2019

I was the jerk who originally removed the cla: human-approved label which used to be our workaround for these situations. Would it be a better idea to reinstitute that, or use /override for this? kubernetes/community#857

FWIW I feel like we also had slightly racey issues with /override for the cla check

@dims
Copy link
Member Author

dims commented Jan 23, 2019

@spiffxp most of the code being brought to this k8s.io/utils repo is from k/k - see the current crop of PR(s) - https://github.com/kubernetes/utils/pulls

i've logged an issue against the CLA bot as well cncf/foundation#26

anything that needs git surgery ends up tripping up the bot.

@spiffxp
Copy link
Member

spiffxp commented Jan 23, 2019

Some of the PR's don't seem to be merging because:

  • the person's e-mail no longer exists (people who have changed employers)
  • the bot in question didn't sign the CLA (k8s-merge-bot)

The former is just not easily fixable

@dims
Copy link
Member Author

dims commented Jan 23, 2019

@spiffxp i am ok with any way as long as its approved by the sig-contribex folks (and documented). i asked this morning and was told that /override was what folks were doing currently.

@spiffxp
Copy link
Member

spiffxp commented Jan 23, 2019

/hold
/approve
I guess I'm leaning "I can't think of anything better, and I trust the repo admins to be responsible".

We could also drop the admins from this repo so it's just the github admin team, but that feels like an unnecessary bottleneck

@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 Jan 23, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, spiffxp

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2019
@BenTheElder
Copy link
Member

This repo has an admins team, so more than just github admins would get /override privileges

That is WAI, repos admins also presumably control what their presubmits are and can manually click merge anyhow?

@spiffxp
Copy link
Member

spiffxp commented Jan 23, 2019

was told that /override was what folks were doing currently

Our hope is that the use of /override is exceptional, and in this case, I feel like migrating code to get it out of tree should... not be considered an exceptional use case. CNCF's CLA bot is failing us here, and I appreciate that you're going to go poke them on the new CLA bot.

I trust you, but I have no way in front of me to mechanically verify that all of the commits in the sundry k/utils PRs are identical to the commits that were submitted under CLA previously.

Seeing spams of /override doesn't give me a lot of confidence that it's going to be used responsibly. I want to say it's your repo, do as you please, I just hope I'm not incurring licensing risk on behalf of the project here.

/hold cancel
/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 Jan 23, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: bc9efca8d05ee3f380767df9f29646f27d7bd5c1

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2019
@spiffxp
Copy link
Member

spiffxp commented Jan 23, 2019

I e-mailed kubernetes-sig-architecture@ for visibility since this is for their repo

@k8s-ci-robot k8s-ci-robot merged commit a9ad40e into kubernetes:master Jan 23, 2019
@k8s-ci-robot
Copy link
Contributor

@dims: Updated the plugins configmap using the following files:

  • key plugins.yaml using file prow/plugins.yaml

In response to this:

k8s.io/utils has a large share of problems with CLA, let's please add the override plugin

Change-Id: Ie9c43a13911493b6a2fc5c1d0934dfef265df582

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.

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. area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants