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 a doc on making PRs easier to review #5232

Merged
merged 1 commit into from
Mar 10, 2015
Merged

Add a doc on making PRs easier to review #5232

merged 1 commit into from
Mar 10, 2015

Conversation

thockin
Copy link
Member

@thockin thockin commented Mar 10, 2015

Too wordy? Open to ideas on making it more succinct. The idea was to have a handy doc we could point people at when they violate these rules.

justinsb
justinsb reviewed Mar 10, 2015
View reviewed changes
docs/devel/faster_reviews.md
your whole PR is pretty trivial, you should instead put your fixups into a new
commit and re-push. Your reviewer can then look at that commit on its own - so
much faster to review than starting over.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with this, but I worry that we're going to end up with a git history that is a mess unless we squash these before merge. Github makes squashing much harder than just hitting the merge button, sadly. (Though it's probably possible to write a small script to do this and merge PRs from the command line).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a note - we should squash these things at the end, if possible. But as long as GitHubs tools suck so badly, this is the only sane way

@thockin thockin force-pushed the devdocs branch 5 times, most recently from e2435d3 to 06aa0a4 Compare March 10, 2015 05:43
@davidopp
Copy link
Member

LGTM

This is awesome!

I wonder if you should add a section on what to do if you've followed all of the guidelines but your PR is still not being reviewed in a timely fashion. The tips I would put there are

  • make sure the oncall assigned it a reviewer/assignee
  • ping the assignee by email if you think you have waited an unreasonable amount of time; we should consider publishing a list of committers' email addresses (work or private, but something that rises above the Github firehose and gets that magic >> gmail marker on it)
  • give some guidance on how to handle the following common situation: various people have chimed in with comments, you've responded to them and think everything is addressed, and you sit and sit and nothing happens. solution here: send an explicit PTAL on the thread, and then if you still don't hear anything after a while, escalate to previous bullet

@thockin
Copy link
Member Author

thockin commented Mar 10, 2015

Good point. Section added. PTAL.

On Tue, Mar 10, 2015 at 12:47 AM, David Oppenheimer <
notifications@github.com> wrote:

LGTM

This is awesome!

I wonder if you should add a section on what to do if you've followed all
of the guidelines but your PR is still not being reviewed in a timely
fashion. The tips I would put there are

  • make sure the oncall assigned it a reviewer/assignee
  • ping the assignee by email if you think you have waited an
    unreasonable amount of time; we should consider publishing a list of
    committers' email addresses (work or private, but something that rises
    above the Github firehose and gets that magic >> gmail marker on it)
  • give some guidance on how to handle the following common situation:
    various people have chimed in with comments, you've responded to them and
    think everything is addressed, and you sit and sit and nothing happens.
    solution here: send an explicit PTAL on the thread, and then if you still
    don't hear anything after a while, escalate to previous bullet

—
Reply to this email directly or view it on GitHub
#5232 (comment)
.

@wojtek-t
Copy link
Member

Awesome doc Tim!

@davidopp
Copy link
Member

Looks good except I wasn't sure about the "fork a new PR" thing. If you do this and then commit, will the comment history on the original PR be lost when you submit?

@derekwaynecarr
Copy link
Member

This is a good doc.

@thockin
Copy link
Member Author

thockin commented Mar 10, 2015

Yes, if you fork a PR you will lose history, but I think it is a viable
option for some situations.
On Mar 10, 2015 10:53 AM, "David Oppenheimer" notifications@github.com
wrote:

Looks good except I wasn't sure about the "fork a new PR" thing. If you do
this and then commit, will the comment history on the original PR be lost
when you submit?

—
Reply to this email directly or view it on GitHub
#5232 (comment)
.

@davidopp
Copy link
Member

LGTM

@alex-mohr alex-mohr self-assigned this Mar 10, 2015
@alex-mohr
Copy link
Contributor

Seems enough consensus -- merging.

alex-mohr added a commit that referenced this pull request Mar 10, 2015
@alex-mohr
Merge pull request #5232 from thockin/devdocs
9d5ee90 
Add a doc on making PRs easier to review
@alex-mohr alex-mohr merged commit 9d5ee90 into kubernetes:master Mar 10, 2015
@thockin thockin deleted the devdocs branch March 19, 2015 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alex-mohr alex-mohr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@thockin @davidopp @wojtek-t @derekwaynecarr @alex-mohr @justinsb @googlebot