Review Request 4333: Propose a github review workflow

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood. Bugs: 3708, 3995 Repository: pants Description Overhaul howto_contribute for a proposed github workflow. Not much to it... primarily deletion. Switch to a github pull request workflow. Recommend that users name the remote for their github fork with their own username. Open issues: How to preserve the "pants-reviews@" list... do we need to? Finding a way to "watch" reviewable PRs by receiving email (this suggests that it isn't possible, unfortunately) Do we need a template for merged PRs, or does the fact that the PR is the default mode of commit make that unnecessary? Update build-support/bin/release-changelog-helper.sh to expand issue/pr links Testing https://travis-ci.org/pantsbuild/pants/builds/169981938 Diffs src/docs/howto_contribute.md (ab5913dcd3e9f17a9c5671043993af7a6482b8ef) View Diff

[Yi Cheng]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Do we need a template for merged PRs, or does the fact that the PR is the default mode of commit make that unnecessary?

When a committer tries to squash merge, the title of the git log will be auto filled with the PR title, but the description will be empty. So as long as the PR description is as good as we expect on RBs, then it should just require the committer to manually copy that into the final merge description.

- Yi Cheng

On October 23rd, 2016, 9:51 p.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 9:51 p.m.

[Mateo Rodriguez]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

On October 23rd, 2016, 5:32 p.m. CDT, Yi Cheng wrote:

Do we need a template for merged PRs, or does the fact that the PR is the default mode of commit make that unnecessary?

When a committer tries to squash merge, the title of the git log will be auto filled with the PR title, but the description will be empty. So as long as the PR description is as good as we expect on RBs, then it should just require the committer to manually copy that into the final merge description.

On October 24th, 2016, 6:33 a.m. CDT, Eric Ayers wrote:

In that case, we should put a template or example of what we want the description to look like in the github PR description
I left comments on https://github.com/pantsbuild/pants/pull/3995 to try out the github review process.

I did the same. But I don't see Eric's comments?

- Mateo

On October 23rd, 2016, 4:51 p.m. CDT, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 4:51 p.m.

[John Sirois]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Thanks for getting this started. I have just notes so far, I've been playing with this a bit in a personal repo.

Testing this out on a personal repo, it seems you can:
1. Get arbitrary diffs between commits - I had not found / tried this before, but works - yay!
2. Easily mess up the squash merge commit. It looks like the default commit message populated is that of the 1st commit in the PR branch. I think not all folks try to polish that 1st commit message, and even if they do, it may need to change. This bit will likely require scripting medium term and great care short term.

src/docs/howto_contribute.md (Diff revision 1)
93	clone url of your fork. Then run the following commands:	87	clone url of your fork and your github username. Then run the following commands:

What's the motivation for this set of changes? I liked origin (personal fork), upstream (gold shared) since it was uniform in spelling for everyone day-2-day and the default when a remote was left out would do the right thing or at least do less potential damage (though we can configure around that mostly, see screen shot I attached to this RBs associated PR).

- John Sirois

On October 23rd, 2016, 3:51 p.m. MDT, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 3:51 p.m.

[Mateo Rodriguez]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

On October 23rd, 2016, 5:32 p.m. CDT, Yi Cheng wrote:

Do we need a template for merged PRs, or does the fact that the PR is the default mode of commit make that unnecessary?

When a committer tries to squash merge, the title of the git log will be auto filled with the PR title, but the description will be empty. So as long as the PR description is as good as we expect on RBs, then it should just require the committer to manually copy that into the final merge description.

On October 24th, 2016, 6:33 a.m. CDT, Eric Ayers wrote:

In that case, we should put a template or example of what we want the description to look like in the github PR description
I left comments on https://github.com/pantsbuild/pants/pull/3995 to try out the github review process.

On October 24th, 2016, 10:57 a.m. CDT, Mateo Rodriguez wrote:

I did the same. But I don't see Eric's comments?

On October 24th, 2016, 11:01 a.m. CDT, Eric Ayers wrote:

@mateo: Looks like I forgot to hit a "submit" button somewhere, can you look again?

Ha, I have been there! I see them now.

- Mateo

On October 23rd, 2016, 4:51 p.m. CDT, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 4:51 p.m.

[Yujie Chen]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

On October 24th, 2016, 2:14 p.m. EDT, John Sirois wrote:

src/docs/howto_contribute.md (Diff revision 1)
93	clone url of your fork. Then run the following commands:	87	clone url of your fork and your github username. Then run the following commands:

What's the motivation for this set of changes? I liked origin (personal fork), upstream (gold shared) since it was uniform in spelling for everyone day-2-day and the default when a remote was left out would do the right thing or at least do less potential damage (though we can configure around that mostly, see screen shot I attached to this RBs associated PR).

+1

- Yujie

On October 23rd, 2016, 5:51 p.m. EDT, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 5:51 p.m.

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

I like it! Just one nit and one suggestion for a naming convention for remotes.

src/docs/howto_contribute.md (Diff revision 1)
122	$ git co master	114	$ git co master

While you're here, can you change this to git checkout, co must be some shortcut the original author of this doc had, but it's not standard git chrome.

src/docs/howto_contribute.md (Diff revision 1)
123	$ git fetch upstream	115	$ git fetch origin

We should probably discourage having the pantsbuild/pants remote be named origin. That seems like a recipe for trouble when you have more than one remote. It's too easy to get confused when our fingers type git push origin master so readily...
Maybe we should require it to be called pantsbuild, and then we can modify the publish scripts to assume that name.

- Benjy Weinberger

On October 23rd, 2016, 9:51 p.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 9:51 p.m. Bugs: 3708, 3995

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Thank you everyone for the feedback. I'm going to focus on https://rbcommons.com/s/twitter/r/4270/ to land it asap, and then I will loop back to apply the feedback here and begin a "beta period".

- Stu Hood

On October 23rd, 2016, 9:51 p.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 9:51 p.m. Bugs: 3708, 3995

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

On October 26th, 2016, 6:43 p.m. UTC, Benjy Weinberger wrote:

src/docs/howto_contribute.md (Diff revision 1)
123	$ git fetch upstream	115	$ git fetch origin

We should probably discourage having the pantsbuild/pants remote be named origin. That seems like a recipe for trouble when you have more than one remote. It's too easy to get confused when our fingers type git push origin master so readily...
Maybe we should require it to be called pantsbuild, and then we can modify the publish scripts to assume that name.

So, my reason for making this change is that I think having a non-standard name for origin is confusing for new contributors (who don't have write access to origin anyway), and is instead more of a power-user maneuver. I expect that committers (who have write access) do not need this page, and will thus not be at risk for accidentally pushing things to the wrong place.

- Stu

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

On October 24th, 2016, 6:14 p.m. UTC, John Sirois wrote:

src/docs/howto_contribute.md (Diff revision 1)
93	clone url of your fork. Then run the following commands:	87	clone url of your fork and your github username. Then run the following commands:

What's the motivation for this set of changes? I liked origin (personal fork), upstream (gold shared) since it was uniform in spelling for everyone day-2-day and the default when a remote was left out would do the right thing or at least do less potential damage (though we can configure around that mostly, see screen shot I attached to this RBs associated PR).

On October 26th, 2016, 2:43 p.m. UTC, Yujie Chen wrote:

+1

Replied to Benjy below; I'd be ok with switching it back, but every new user I've interacted who has followed this workflow has ended up confused by it.

- Stu

On October 23rd, 2016, 9:51 p.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Oct. 23, 2016, 9:51 p.m.

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

On October 26th, 2016, 6:43 p.m. UTC, Benjy Weinberger wrote:

src/docs/howto_contribute.md (Diff revision 1)
123	$ git fetch upstream	115	$ git fetch origin

We should probably discourage having the pantsbuild/pants remote be named origin. That seems like a recipe for trouble when you have more than one remote. It's too easy to get confused when our fingers type git push origin master so readily...
Maybe we should require it to be called pantsbuild, and then we can modify the publish scripts to assume that name.

On October 27th, 2016, 8:11 p.m. UTC, Stu Hood wrote:

So, my reason for making this change is that I think having a non-standard name for origin is confusing for new contributors (who don't have write access to origin anyway), and is instead more of a power-user maneuver. I expect that committers (who have write access) do not need this page, and will thus not be at risk for accidentally pushing things to the wrong place.

Reading more feedback, it sounds like this should go in a separate change, which is fine with me.

- Stu

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Nov. 18, 2016, 8:07 p.m. Changes Reverted the upstream/origin change, and applied review feedback from the PR.

Bugs: 3708, 3995 Repository: pants

Description (updated)

Overhaul howto_contribute for a proposed github workflow. Not much to it... primarily deletion.

Switch to a github pull request workflow. Recommend that users name the remote for their github fork with their own username.

Added PULL_REQUEST_TEMPLATE.md to help encourage more useful PR messages. Added a blurb about being thoughtful/clear in comments as to which issues are blockers, and which are not. (independent of this change) Configured the pantsbuild/pants repo to only allow squash merges in PRs. Removed the rbt scripts. Testing (updated) https://travis-ci.org/pantsbuild/pants/builds/177117555 Diffs (updated) PULL_REQUEST_TEMPLATE.md (PRE-CREATION) rbt (de81e7ea7311fc368306dec1a6c5cadd87ef41d1) rbt-create (70d86794a5595430af7f24d31cf517a4417f0ce5) rbt-update (5d55d3a204ec87d83cfed11639f05d3dd9f151e9) src/docs/howto_contribute.md (ab5913dcd3e9f17a9c5671043993af7a6482b8ef) View Diff Show Changes

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Nov. 18, 2016, 8:11 p.m.

Bugs: 3708, 3995 Repository: pants Description (updated)

Overhaul howto_contribute for a proposed github workflow. Not much to it... primarily deletion.

Switch to a github pull request workflow.

Added PULL_REQUEST_TEMPLATE.md to help encourage more useful PR messages. Added a blurb about being thoughtful/clear in comments as to which issues are blockers, and which are not. (independent of this change) Configured the pantsbuild/pants repo to only allow squash merges in PRs. Removed the rbt scripts.

Testing https://travis-ci.org/pantsbuild/pants/builds/177117555 Diffs

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Ship it!

Ship It!

- Benjy Weinberger

On November 18th, 2016, 8:11 p.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood. Updated Nov. 18, 2016, 8:11 p.m. Bugs: 3708, 3995 Repository: pants Description

Overhaul howto_contribute for a proposed github workflow. Not much to it... primarily deletion.

Switch to a github pull request workflow. Added PULL_REQUEST_TEMPLATE.md to help encourage more useful PR messages. Added a blurb about being thoughtful/clear in comments as to which issues are blockers, and which are not. (independent of this change) Configured the pantsbuild/pants repo to only allow squash merges in PRs. Removed the rbt scripts.

[John Sirois]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4333/

Ship it!

PULL_REQUEST_TEMPLATE.md (Diff revision 2)
		1	### Problem

Not a fan of this recipe, but its not exactly mandated either so I won't block on it as a suggestion.

- John Sirois

On November 18th, 2016, 1:11 p.m. MST, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Chris Heisterkamp, John Sirois, Kris Wilson, Mateo Rodriguez, and Eric Ayers. By Stu Hood.

Updated Nov. 18, 2016, 1:11 p.m.

Bugs: 3708, 3995 Repository: pants Description

Overhaul howto_contribute for a proposed github workflow. Not much to it... primarily deletion.

Switch to a github pull request workflow. Added PULL_REQUEST_TEMPLATE.md to help encourage more useful PR messages. Added a blurb about being thoughtful/clear in comments as to which issues are blockers, and which are not. (independent of this change) Configured the pantsbuild/pants repo to only allow squash merges in PRs. Removed the rbt scripts.