Skip to content
Permalink
Browse files
Move from wiki: CommitterWorkfow and ProcessingPullRequests
  • Loading branch information
agrieve committed Mar 13, 2014
1 parent f7f027f commit 8857f71f65e4ba968f4bca850bb752040c9b9e37
Show file tree
Hide file tree
Showing 3 changed files with 267 additions and 0 deletions.
@@ -0,0 +1,22 @@
# Code Reviews

TODO: Review Board didn't really work out :(. Come up with something better.

* Use [[http://reviews.apache.org/|reviews.apache.org]] to create a review request.
* You will need a Review Board userid and password. One can be requested from the site.
* By default, the ML will be notified of your review request.
* If you have someone in particular that you would like approval from, be sure to add them in "People" field of the review.
* To create your review request is to use [[http://www.reviewboard.org/docs/rbtools/dev/|post-review]] (RBTools) from the repo with the change, only if the repo contains the file '.reviewboardrc'
Currently the following repos contain .reviewboardrc:
cordova-coho
cordova-cli
cordova-android
cordova-ios
cordova-js

* If you don't want to use post-review tool, then on the web site:
* Click "New Review Request"
* On your workstation do a git diff and pipe the output to a file. In the new request, select the git repo, the base dir where you did the diff, upload the diff file you created, and click Create.
* On the next web page, fill in the description text, the name of the Branch (i.e., "master"), the Bug (i.e., "CB-4960"), the Description and the Testing Done fields. If you want a review from a specific person, enter their userid / name in the People field. If you want input from the whole community, enter "cordova" in the Groups field. Click the Submit button.
* After you have received sufficient feedback, click the button to mark your review as Discarded or Submitted [to a stream].
* If you don't want to use !ReviewBoard you can use Github Pull Request
@@ -0,0 +1,155 @@
# Committer Workflow

## One-time Steps

Congratulations! You've gained the confidence of your fellow Cordova committers
and have been granted the ability to commit code directly, and to apply pull
requests from others. You should receive an email from our Apache mentor with
the details of how to setup your account.

### Configure your git repos

It's convenient to have the origin of your git repos to point to Apache's repos
(as opposed to your clone of them on github). The easiest way to do this is to
delete them and re-clone them using coho:

git clone https://git-wip-us.apache.org/repos/asf/cordova-coho.git
cd cordova-coho
npm install
cd ..
cordova-coho/coho repo-clone -r plugins -r mobile-spec -r ...

Test out your credentials with the following:

git pull
git push

If all goes well, git push should have asked you for your username and password, and an "Everything up-to-date" message should have been printed.

### Join the private mailing-list

This is a list that only committers can join.

Send an email to `private-subscribe@cordova.apache.org`.

Note that this is a moderated list, so your request to join must be manually accepted.

### Do Your Homework

Read through: http://www.apache.org/dev/committers.html

# Commit Workflow

### Step 1: Mail the Mailing-list (_optional_)
This is required if any of the following apply:
* Your change will add/remove/change a public Cordova API
* You suspect that your change has a chance of being controversial
* You would like feedback before you begin

When possible, try to phrase things in the form of a proposal. If no one objects (within a workday or two), then consider yourself to have [lazy consensus](http://www.apache.org/foundation/glossary.html#LazyConsensus).

### Step 2: Ensure there is a JIRA issue
* They are not necessary for *all* changes, (e.g. style nits, refactoring)
* They should always be used for new features and bugs

### Step 3: Create a topic branch (_optional_)
* Using a public topic branch is necessary only when you would like to collaborate on the feature.
* For small changes, private topic branches are preferred.
* Note: You should never rebase a public topic branch!

### Step 4: Make your changes
* Thank you for making the world a better place.

### Step 5: Test your changes ###
* You are responsible for testing the commits you push.
* Tests vary by repo, but in general:
* Plugins: Automated tests in mobile-spec and/or manual tests in mobile spec
* Tools: run `npm test` from the project root
* Platforms: Native unit tests (i.e., `cordova-android/test`, `cordova-ios/CordovaLibTests`)
* Cordova JS: Run `grunt test`
* If there is no existing test that exercises your code, consider adding one
* If you are writing documentation (i.e., cordova-docs), be aware of the [style guidelines](https://github.com/apache/cordova-docs/blob/master/STYLESHEET.md|style guidelines).

### Step 6: Ask for a code review (_optional_)
* Do this if you want a second pair of eyes to look at your code before it goes in.
* Use either [reviewboard](code-reviews.md) or a GitHub pull request.

### Step 7: Push your change
* When possible, rebase & squash your commits
* Make sure you can figure out what your commit does by the first line of your commit discription.
* If it fixes a regression, then also cherry-pick it into the appropriate release branch.
* Here is an example workflow for committing a change when you've made it on a topic branch


git pull
git checkout topic_branch
git rebase master -i
git checkout master
git merge --ff-only topic_branch
git push
git branch -d topic_branch

* Here is an example workflow for committing a change when you've made it on master:


git pull --rebase
git rebase origin/master -i # Squash & reword commit messages
git push

* If you ever end up with a merge commit on master that you don't want:


git rebase origin/master

* If you need to add your change to a release branch:


git checkout 2.9.x
git cherry-pick -x COMMIT_HASH # the -x flag adds "cherry-picked from <commit>" to the commit messages
git push origin 2.9.x

The `git rebase -i` step is your chance to clean up the commit messages and to combine small commits when appropriate. For example:

Commit A: CB-1234 Implemented RockOn feature
Commit B: CB-1234 Added tests for RockOn
Commit C: Fixed RockOn not working with empty strings
Commit D: Renamed RockOn to JustRock
Commit E: Refactor MainFeature to make use of JustRock.

* In this case, it would be appropriate to combine commits A-D into a single commit, or at least commits A & C.
* Fewer commits are better because:
* Easier to comprehend the diff
* Makes changelog more concise
* Easier to roll-back commits should the need arise
* __NEVER REBASE A COMMIT THAT HAS BEEN PUSHED__
* For all commits:
* Prefix with JIRA IDs: CB-1234
* For commits to cordova-js or to plugins:
* Prefix the message with the affected_platform so that it's clear who should take interest in the commit
* e.g.: `CB-1234 android: Improved exec bridge by using strings instead of JSON`
* e.g.: `CB-1234 all: Fixed plugin loading paths that start with /`

### Step 8: Update JIRA
* An Apache bot should have already added a comment to the issue with your commit ID (based on the CB-1234 being in the commit message).
* Click the "Resolve Issue" button
* Add a comment saying what version the commit is in (e.g. Fixed in 0.1.3-dev).

# Which Branch to Commit To

### Platforms, mobile-spec, cordova-js, cordova-docs:
* Commit all changes to branch: `master`
* If it fixes a regression, cherry-pick into the release branch (see CuttingReleases)
* e.g. To cherry pick the last commit from master: `git cherry-pick -x master`

### Plugins
* Commit all changes to branch: `dev`
* Through '''Cordova 3.0''', `plugman` installed from `master`, which means we couldn't use `master` for development.
* Post '''Cordova 3.0''', `plugman` has support for a registry...

### All other Repos:
* Commit all changes to branch: `master`

# Processing Pull Requests #

See [processing-pull-requests.md](processing-pull-requests.md)

@@ -0,0 +1,90 @@
# Processing Pull Requests

## Prerequisites:
* Ensure you are familiar with [committer-workflow.md](committer-workflow.md)

## Step 0:
* Find what requests need attention by looking a the GitHub page.
* To look at them in aggregate:


./cordova-coho/coho list-pulls | tee pulls.list | less -R

* To filter out those that you last commented on:


./cordova-coho/coho list-pulls --hide-user=agrieve

* To show only certain repos:


./cordova-coho/coho list-pulls -r js -r android -r plugin-inappbrowser

## Step 1: Review the change (part 1)
* Ensure that we actually want the change (if unsure, bring it up on the ML)
* If there is no JIRA issue for the change, create one
* Ensure the JIRA issue has a link to the pull request
* Ensure the pull request has a link to the JIRA issue
* View the user's branch in github and request changes be made (if applicable) by adding comments in the web interface

## Step 2: Ensure they have signed the Contributor Agreement
* For trivial changes, this is not necessary
* Find their name on: https://people.apache.org/committer-index.html#unlistedclas
* If it is not there, respond with:

_Thanks for the pull request. I've had a look at it and think it looks good. Before we can merge it though, you need to sign Apache's Contributor License Agreement (can be done online): http://www.apache.org/licenses/#clas_

## Step 3: Merge the change
Run the following as an exemplary way to merge to master:

git pull https://github.com/GitHubUser/cordova-FOO BRANCH
git rebase origin/MASTER_OR_DEV -i

The rebase step will let you interactively review the changes being made to master. You should:

* Squash as many commits as is reasonable together.
* Re-write commit messages to include JIRA issue numbers in the form CB-#### (no "[]"'s, no ":")
* In the final commit message (if there are multiple), [tell GitHub to close the pull request](https://help.github.com/articles/closing-issues-via-commit-messages)

Example:

CB-6124 Make `cordova plugin remove` resilient to a missing plugin directory

This breaks a couple prepare tests that Andrew deletes in the next commit.

github: close #57

## Step 4: Check the author

Git keeps track of the author of commits separately from the committer of the commit.

Typically, both of these values are set to the person submitting the pull request.
After your pull/merge/rebase/whatever work, you may find the values have changed - or not.
What we would typically be looking for is the final commit to have YOU as the committer and the original author as the author.

You can check these fields with the following command:

git log --pretty=full

If the author is set to YOU, and you'd like to reset it to the original author, you can amend the commit:

git commit --amend --author=some_author_id_here

## Step 5: Review the change (part 2)
* You are responsible for testing the changes that push.
* Refer to [committer-workflow.md](committer-workflow.md) for how to test each repo.
* If it would be appropriate to write a test
* Either write one yourself, or ask the author to do so.
* If you write one yourself, commit it in a follow-up (don't squash it with theirs)

## Step 6: Push the change

git push

## Step 7: Update JIRA
* Same as you would as if you authored the change.

## Step 8: Final details
* The commit will get attached to the pull request automatically from the "closes #" in the commit message.
* If you haven't done so already, thank them for their contribution via a pull request comment :).

0 comments on commit 8857f71

Please sign in to comment.