Reviewing and merging a pull request¶
Of course it’s not possible to give an exact recipe for reviewing a pull request, you simply have to assess the code and decide whether you’re happy with it. Nonetheless, here is an incomplete list of things to look for:
Does the pull request contain one logically separate piece of work (e.g. one new feature, bug fix, etc. per pull request)?
Does the pull request follow the guidelines for writing commit messages?
Is the branch up to date - have the latest commits from master been pulled into the branch?
Does the pull request contain new or updated tests for any new or updated code, and do the tests follow CKAN’s testing coding standards?
Do all the CKAN tests pass, on the new branch?
Does the pull request contain new or updated docs for any new or updated features, and do the docs follow CKAN’s documentation guidelines?
If the new code contains changes to the database schema, does it have a database migration?
Does the code contain any changes that break backwards-incompatibility? If so, is the breakage necessary or do the benefits of the change justify the breakage? Have the breaking changes been added to the changelog?
Backwards-compability needs to be considered when making changes that break the interfaces that CKAN provides to third-party code, including API clients, plugins and themes.
Does the new code add any dependencies to CKAN (e.g. new third-party Python modules imported)? If so, is the new dependency justified and has it been added following the right process? See Upgrading CKAN’s dependencies.
Merging a pull request¶
Once you’ve reviewed a pull request and you’re happy with it, you need to
merge it into the master branch. You should do this using the
option in the
git merge command. For example:
git checkout feature-branch
git pull origin feature-branch
git checkout master
git pull origin master
git merge --no-ff feature-branch
git push origin master
Before doing the
git push, it’s a good idea to check that all the tests are
passing on your master branch (if the latest commits from master have already
been pulled into the feature branch on github, then it may be enough to check
that all tests passed for the latest commit on this branch on
Also before doing the
git push, it’s a good idea to use
git log and/or
git diff to check the difference between your local master branch and the
remote master branch, to make sure you only push the changes you intend to
git log ...origin/master
git diff ..origin/master