Propose a github review workflow

Review Request #4333 — Created Oct. 23, 2016 and submitted

stuhood
pants
3708, 3995
pants-reviews
benjyw, cheister, jsirois, kwlzn, mateor, zundel

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.

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

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  1. 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.

    1. 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.

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

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

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

  2. 
      
  1. 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.
  2. src/docs/howto_contribute.md (Diff revision 1)
     
     
    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. +1

    2. 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.

  3. 
      
  1. thanks for working on this! lgtm w/ a few thoughts; overall, I think having a streamlined PR process here will be excellent for all involved - no more $$ spent on rbcommons, more intuitive non-committer contributions, less pages/linkages to maintain, first class CI checks etc w/o updating travis links and more.

  2. src/docs/howto_contribute.md (Diff revision 1)
     
     
     

    here before your change, but maybe:

    s/Make a note the/Note the/ ?
    
  3. src/docs/howto_contribute.md (Diff revision 1)
     
     
     
     
     

    I wonder if it would make sense to standardize on a static remote name scheme that everyone could share?

    e.g. git remote add fork <url-to-clone-your-fork>?

    that way instead of having to coach ppl toward git push <username> branch we can have a standard git push fork branch etc.

  4. src/docs/howto_contribute.md (Diff revision 1)
     
     
     

    do you know if setting the assignees field for a PR will trigger an email to the assignees themselves to take a look?

    I don't recall ever seeing an "you've been assigned an issue" email from github - but hopefully they at least show up as lightweight notifications on the github web UI?

  5. src/docs/howto_contribute.md (Diff revision 1)
     
     
     
     
     

    iirc the release tooling relies on parsing of the RB commit format from a git log for auto-extraction of metadata - I suppose we'll need to adapt that accordingly?

  6. 
      
  1. 
      
  2. src/docs/howto_contribute.md (Diff revision 1)
     
     
     
     
     

    It seems distracting that reformatting this is the controversial part of the review when the main thrust of the review is to change from rbcommons to git. Leaving your own repo at origin makes some things less confusing, naming it the new way has other advantages. Let's split that out into a separate review.

    The existing instructions reflect github's advice on forking a repo at https://help.github.com/articles/fork-a-repo/

    So while I can see that this saves some onboarding, I don't think it makes it simpler for pants users that don't know how to fork a repo in git with their own repository.

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

  2. src/docs/howto_contribute.md (Diff revision 1)
     
     

    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.

  3. src/docs/howto_contribute.md (Diff revision 1)
     
     

    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.

    1. 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.

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

  4. 
      
  1. 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".

  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as e35dc2eebe15b08be2a510edf77090d0c01d8bd0

  1. 
      
  2. PULL_REQUEST_TEMPLATE.md (Diff revision 2)
     
     
    Not a fan of this recipe, but its not exactly mandated either so I won't block on it as a suggestion.
  3. 
      
Loading...