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 closure guidelines #15261

Merged
merged 1 commit into from
Jul 14, 2019
Merged

Add closure guidelines #15261

merged 1 commit into from
Jul 14, 2019

Conversation

zacharysarah
Copy link
Contributor

This PR adds guidelines for PR wranglers on when to close PRs.

/sig docs
/assign @cody-clark

@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 3, 2019
@netlify
Copy link

netlify bot commented Jul 3, 2019 •
edited
Loading

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 0148288

https://deploy-preview-15261--kubernetes-io-master-staging.netlify.com

irvifa
irvifa reviewed Jul 3, 2019
View reviewed changes
content/en/docs/contribute/advanced.md

Don't be afraid to close pull requests. Contributors can easily reopen and resume works in progress. Oftentimes a closure notice is what spurs an author to resume and finish their contribution.

To close a pull request, leave a `/close` comment on the PR.
Copy link
Member

Choose a reason for hiding this comment

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

In case that we need the PR can we become a co-author for the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

In case that we need the PR can we become a co-author for the PR?

If you're asking whether or not you can "take over" the PR by pulling the commit locally, working on it, and pushing up a different PR, the answer is yes, of course, but it is best to coordinate with the original author to ensure they have actually abandoned their efforts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@irvifa Sure. Would you agree, though, that co-authoring is out of scope for this particular PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I agree for that. Thanks 🙁 Another case if we can't really ping the previous author what can be done?

Copy link
Contributor Author

@zacharysarah zacharysarah Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@irvifa In that case cherry-picking seems like a reasonable choice. You can always /reopen the PR to continue as a coauthor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aside In a couple of cases I have adopted commits from another author's PR (and communicated about this). I definitely wouldn't adopt commits from an author who had not yet signed the CLA, because the subsequent pull request would be on shaky ground in terms of licensing.

@cody-clark
Copy link
Contributor

Should we mention lifecycle/rotten, too?

Preview: https://deploy-preview-15261--kubernetes-io-master-staging.netlify.com/docs/contribute/advanced/#when-to-close-pull-requests

@zacharysarah
Copy link
Contributor Author

@cody-clark 👋

Should we mention lifecycle/rotten, too?

Sure, I'll add a reference.

@irvifa
Copy link
Member

irvifa commented Jul 4, 2019

/lgtm from me

@zacharysarah
Copy link
Contributor Author

@cody-clark If you're happy with this, could I please get an /lgtm and /approve? 😄

@tengqm
Copy link
Contributor

tengqm commented Jul 6, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2019
kbhawkey
kbhawkey reviewed Jul 8, 2019
View reviewed changes
content/en/docs/contribute/advanced.md
To close a pull request, leave a `/close` comment on the PR.

{{ <note> }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @zacharysarah.
nit: The note shortcode should get corrected:
{{< note >}}
{{< /note >}}

Copy link
Contributor Author

@zacharysarah zacharysarah Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@kbhawkey Thanks! Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbhawkey 👋 If this looks good to you, please feel free to LGTM/approve. 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbhawkey Remember you can /approve now, too. 😄

Add closure guidelines
0148288 
Add close command

Add a note about fejta-bot

Feedback from kbhawkey
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2019
@kbhawkey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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 Jul 14, 2019
@k8s-ci-robot k8s-ci-robot merged commit ce8fc52 into kubernetes:master Jul 14, 2019
@zacharysarah zacharysarah deleted the closing-prs branch July 14, 2019 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaypipes jaypipes jaypipes left review comments

@kbhawkey kbhawkey kbhawkey left review comments

@irvifa irvifa irvifa left review comments

@sftim sftim sftim left review comments

@jimangel jimangel Awaiting requested review from jimangel

@zhangxiaoyu-zidif zhangxiaoyu-zidif Awaiting requested review from zhangxiaoyu-zidif

Assignees

@kbhawkey kbhawkey

@tengqm tengqm

@cody-clark cody-clark

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zacharysarah @cody-clark @irvifa @tengqm @kbhawkey @k8s-ci-robot @jaypipes @sftim