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

Update CONTRIBUTING.md advice on topic branches #11720

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
4 participants
@wvu-r7
Copy link
Contributor

wvu-r7 commented Apr 12, 2019

This updates the verbiage in CONTRIBUTING.md with our verified reasons for enforcing topic branches.

We never verified why GitHub diffs can get messed up over time. I'm partly to blame for perpetuating the line that it was a master PR's fault. I'll continue to look into that, but until we figure it out, let's clarify our verified reasons for using a topic branch. You can still access master-branch commit history and diffs via the command line.

FWIW, there was no technical reason a master PR couldn't be merged, so long as the contributor kept to one PR at a time. Obviously this is self-limiting and messy, which is the primary reason we introduced this recommendation.

We wanted contributors to learn the best practice, so closing their PR was the approach we took. The only drawback we've noted is that review history should be cross-referenced between PRs. This can be as simple as tagging the old PR's number in the new PR's description.

Words from @jmartin-r7.

30fec1a, 3bbb2d6, #9605 (comment)

#11711

@wvu-r7 wvu-r7 requested review from bcoles and jmartin-r7 Apr 12, 2019

@bcoles

This comment has been minimized.

Copy link
Contributor

bcoles commented Apr 12, 2019

Relevant #11224

@jmartin-r7 jmartin-r7 merged commit 8fcb6ad into rapid7:master Apr 12, 2019

3 checks passed

Metasploit Automation - Sanity Test Execution Successfully completed all tests.
Details
Metasploit Automation - Test Execution Successfully completed all tests.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jmartin-r7 added a commit that referenced this pull request Apr 12, 2019

msjenkins-r7 added a commit that referenced this pull request Apr 12, 2019

@wvu-r7 wvu-r7 removed request for bcoles and jmartin-r7 Apr 12, 2019

@wvu-r7

This comment has been minimized.

Copy link
Contributor Author

wvu-r7 commented Apr 12, 2019

Thanks! Another blow has been dealt to the Metasploit cargo cult. :)

@wvu-r7 wvu-r7 deleted the wvu-r7:feature/contributing branch Apr 12, 2019

@timwr

This comment has been minimized.

Copy link
Contributor

timwr commented Apr 13, 2019

I was never 100% why we require the contributors not to send a pull request from master. While it's not best practice I'd prefer we just warn them, rather than close the pull request and request a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.