[
https://issues.apache.org/jira/browse/BEAM-6122?focusedWorklogId=171952&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-171952
]
ASF GitHub Bot logged work on BEAM-6122:
----------------------------------------
Author: ASF GitHub Bot
Created on: 04/Dec/18 16:59
Start Date: 04/Dec/18 16:59
Worklog Time Spent: 10m
Work Description: tweise closed pull request #7129: [BEAM-6122] Update
committer guidelines
URL: https://github.com/apache/beam/pull/7129
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/website/src/contribute/committer-guide.md
b/website/src/contribute/committer-guide.md
index 20e2f8b4f71f..2fa9b4c4bd53 100644
--- a/website/src/contribute/committer-guide.md
+++ b/website/src/contribute/committer-guide.md
@@ -24,6 +24,24 @@ This guide is for
[committers](https://www.apache.org/foundation/how-it-works.html#committers)
and covers Beam's guidelines for reviewing and merging code.
+## Pull request review objectives
+
+The review process aims for:
+
+* Review iterations should be efficient, timely and of quality (avoid tiny or
out-of-context changes or huge mega-changes)
+* Support efficiency of authoring (don't want to wait on a review for a tiny
bit because GitHub makes it very hard to stack up reviews in sequence / don't
want to have major changes blocked because of difficulty of review)
+* Ease of first-time contribution (encourage to follow [contribution
guildelines](/contribute/#contributing-code)
+ but committer may absorb some extra effort for new contributors)
+* Pull requests and commit messages establish a clear history with purpose and
origin of changes
+* Ability to perform a granular rollback, if necessary (also see
[policies](/contribute/postcommits-policies/))
+
+Granularity of changes:
+
+* We prefer small independent, incremental PRs with descriptive, isolated
commits. Each commit is a single clear change
+* It is OK to keep separate commits for different logical pieces of the code,
if they make reviewing and revisiting code easier
+* Making commits isolated is a good practice, authors should be able to
relatively easily split the PR upon reviewer's request
+* Generally, every commit should compile and pass tests.
+
## Always get to LGTM ("Looks good to me!")
After a pull request goes through rounds of reviews and revisions, it will
@@ -77,16 +95,27 @@ At some point in the review process, the change to the
codebase will be
complete. However, the pull request may have a collection of review-related
commits that are not meaningful to preserve in the history. The reviewer should
give the LGTM and then request that the author of the pull request rebase,
-squash, split, etc, the commits, so that the history is most useful. Favor
-commits that do just one thing. The commit is the smallest unit of easy
+squash, split, etc, the commits, so that the history is most useful:
+* Favor commits that do just one thing. The commit is the smallest unit of easy
rollback; it is easy to roll back many commits, or a whole pull request, but
harder to roll back part of a commit.
+* Commit messages should tag JIRAs and be otherwise descriptive.
+It should later not be necessary to find a merge or first PR commit to find
out what caused a change.
+* Squash the "Fixup!", "Address comments" type of commits that resulted from
review iterations.
## Merging it!
+While it is preferred that authors squash commits after review is complete,
+there may be situations where it is more practical for the committer to handle
this
+(such as when the action to be taken is obvious or the author isn't available).
+The committer may use the "Squash and merge" option in Github (or modify the
PR commits in other ways).
+The committer is ultimately responsible and we "trust the committer's
judgment"!
+
After all the tests pass, there should be a green merge button at the bottom of
-the pull request. There are multiple choices and you should choose "Merge pull
-request" (the default). This preserves the commit history and adds a merge
+the pull request. There are multiple choices. Unless you want to squash commits
+as part of the merge (see above) you should choose "Merge pull
+request" and ensure "Create a merge commit" is selected from the drop down.
+This preserves the commit history and adds a merge
commit, so be sure the commit history has been curated appropriately.
Do _not_ use the default GitHub commit message, which looks like this:
diff --git a/website/src/contribute/index.md b/website/src/contribute/index.md
index 3cf21bd341fd..ec2351808fa7 100644
--- a/website/src/contribute/index.md
+++ b/website/src/contribute/index.md
@@ -149,9 +149,11 @@ To contribute code, you need
have an open source license
[compatible](https://www.apache.org/legal/resolved.html#criteria) with Apache.
1. Add unit tests for your change
1. When your change is ready to be reviewed and merged, create a pull request.
- Format the pull request title like `[BEAM-XXX] Fixes bug in
ApproximateQuantiles`,
+ Format commit messages and the pull request title like `[BEAM-XXX] Fixes
bug in ApproximateQuantiles`,
where you replace BEAM-XXX with the appropriate JIRA issue.
This will automatically link the pull request to the issue.
+ Use descriptive commit messages that make it easy to identify changes and
provide a clear history.
+ To support efficient and quality review, avoid tiny or out-of-context
changes and huge mega-changes.
1. The pull request and any changes pushed to it will trigger [pre-commit
jobs](/contribute/testing/). If a test fails and appears unrelated to your
change, you can cause tests to be re-run by adding a single line comment on
your
@@ -163,7 +165,7 @@ To contribute code, you need
.testinfra/jenkins, but use these sparingly because post-commit
tests consume shared development resources.
1. Pull requests can only be merged by a
- [beam committer]({{ site.baseurl }}/contribute/team/).
+ [Beam committer]({{ site.baseurl }}/contribute/team/).
To find a committer for your area, either:
- look in the OWNERS file of the directory where you changed files, or
- look for similar code merges, or
@@ -172,6 +174,9 @@ To contribute code, you need
Use `R: @username` in the pull request to notify a reviewer.
1. If you don't get any response in 3 business days, email the dev@ list to
ask for someone to look at your pull
request.
+1. Review feedback typically leads to follow-up changes. You can add these
changes as additional "fixup" commits to the
+ existing PR/branch. This will allow reviewer(s) to track the incremental
progress. After review is complete and the
+ PR accepted, multiple commits should be squashed (see [Git workflow
tips](https://cwiki.apache.org/confluence/display/BEAM/Git+Tips)).
## When will my change show up in an Apache Beam release?
@@ -190,28 +195,28 @@ unassigned from the author but will stay open.
## Accounts and Permissions
-- [Beam issue tracker
(JIRA)](https://issues.apache.org/jira/projects/BEAM/issues)
- anyone can access it and browse issues. Anyone can register an account and
login
+- [Beam issue tracker
(JIRA)](https://issues.apache.org/jira/projects/BEAM/issues):
+ Anyone can access it and browse issues. Anyone can register an account and
login
to create issues or add comments. Only contributors can be assigned issues.
If
you want to be assigned issues, a PMC member can add you to the project
contributor
group. Email the [dev@ mailing list]({{ site.baseurl
}}/community/contact-us)
to ask to be added as a contributor in the Beam issue tracker, and include
your ASF Jira username.
-- [Beam Wiki
Space](https://cwiki.apache.org/confluence/display/BEAM/Apache+Beam).
- If you wish to contribute changes, please request edit access on the
- [dev@ mailing list]({{ site.baseurl }}/community/contact-us).
+- [Beam Wiki
Space](https://cwiki.apache.org/confluence/display/BEAM/Apache+Beam):
+ Anyone has read access. If you wish to contribute changes, please create an
account and request edit access on the
+ [dev@ mailing list]({{ site.baseurl }}/community/contact-us) (include your
Wiki account user ID).
- Pull requests can only be merged by a
- [beam committer]({{ site.baseurl }}/contribute/team/).
+ [Beam committer]({{ site.baseurl }}/contribute/team/).
-- [Voting on a release](https://www.apache.org/foundation/voting.html).
Everyone can vote. Only
+- [Voting on a release](https://www.apache.org/foundation/voting.html):
Everyone can vote. Only
[Beam PMC]({{ site.baseurl }}/contribute/team/) members should mark their
votes as binding.
## Communication
All communication is expected to align with the [Code of
Conduct](https://www.apache.org/foundation/policies/conduct).
-Discussions about contributing code to beam happens on the [dev@ mailing
list]({{ site.baseurl
+Discussion about contributing code to Beam happens on the [dev@ mailing
list]({{ site.baseurl
}}/community/contact-us/). Introduce yourself!
Questions can be asked on the [#beam channel of the ASF slack]({{ site.baseurl
diff --git a/website/src/contribute/postcommits-guides.md
b/website/src/contribute/postcommits-guides.md
index 07faab4d831b..78b064e99dcd 100644
--- a/website/src/contribute/postcommits-guides.md
+++ b/website/src/contribute/postcommits-guides.md
@@ -38,7 +38,7 @@ Rolling back is usually the fastest way to fix a failing
test. However it is
is often inconvenient for the original author. To help the author fix the
issue, follow these steps when you rollback someone's change.
-1. Rollback the PR.
+1. Rollback the PR (or individual commit of the PR). The rollback PR should
be green except in rare cases.
1. Create a JIRA issue that contains the following information:
* the reason for the rollback
* a link to the test failure's JIRA issue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 171952)
Time Spent: 5h 50m (was: 5h 40m)
> Update committer guidelines
> ---------------------------
>
> Key: BEAM-6122
> URL: https://issues.apache.org/jira/browse/BEAM-6122
> Project: Beam
> Issue Type: Task
> Components: website
> Reporter: Thomas Weise
> Assignee: Thomas Weise
> Priority: Major
> Time Spent: 5h 50m
> Remaining Estimate: 0h
>
> Per discussion in
> [https://lists.apache.org/thread.html/6d922820d6fc352479f88e5c8737f2c8893ddb706a1e578b50d28948@%3Cdev.beam.apache.org%3E]
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
