7. Contributor’s guide¶
This section is notes on contributing to Charliecloud development. Currently, it is messy and incomplete. Patches welcome!
It documents public stuff only. If you are on the core team at LANL, also consult the internal documentation and other resources.
Note
We’re interested in and will consider all good-faith contributions. While it does make things easier and faster if you follow the guidelines here, they are not required. We’ll either clean it up for you or walk you through any necessary changes.
7.1. Workflow¶
7.1.1. Branching model¶
- We try to keep the branching model simple. Right now, we’re pretty similar to Scott Chacon’s “GitHub Flow”: Master is stable; work on short-lived topic branches; use pull requests to ask for merging.
- Tagged versions currently get more testing. We are working to improve testing for normal commits on the master, but full parity is probably unlikely.
- Don’t work directly on master. Even the project lead doesn’t do this. While it may appear that some trivial fixes are being committed to the master directly, what’s really happening is that these are prototyped on a branch and then fast-forward merged after Travis passes.
- Keep history tidy. While it’s often difficult to avoid a branch history with commits called “try 2” and “fix Travis”, clean it up before submitting a PR. Interactive rebase is your friend.
- Feature branches should generally be rebased, rather than merged into, in order to track master. PRs with conflicts will generally not be merged.
- Feature branches are merged with either a merge commit or squash and rebase,
which squashes all the branch’s commits into one and then rebases that
commit onto master’s HEAD (called “squash and merge” by GitHub). This can be
done either on the command line or in the GitHub web interface.
- Merge commit message example:
merge PR #268 from @j-ogas: remove ch-docker-run (closes #258)
- Squash and rebase commit message example:
PR #270 from me: document locations of .bats files
- Merge commit message example:
- Feature branches in the main repo are deleted by the project lead after merging.
- Remove obsolete remote branches from your repo with
git fetch --prune --all
.
7.1.2. Issues, pull requests, and milestones¶
We use milestones to organize what is planned for, and what actually happened, in each version.
All but the most trivial changes should have an issue or a pull request (PR). The relevant commit message should close the issue (not PR) using the GitHub syntax.
The standard workflow is:
- Propose a change in an issue.
- Tag the issue with its kind (bug, enhancement, question).
- Get consensus on what to do and how to do it, with key information recorded in the issue.
- Assign the issue to a milestone.
- Submit a PR that refers to the issue.
- Review/iterate.
- Project lead merges. No one other than the project lead should be merging to or committing on master.
Don’t tag or milestone the PR in this case, so that the change is only listed once in the various views.
Bare PRs with no corresponding issue are also considered but should have reached consensus using other means, which should be stated in the PR. Tag and milestone the PR.
We acknowledge submitted issues by tagging them.
Issues and PRs should address a single concern. If there are multiple concerns, make separate issues and/or PRs. For example, PRs should not tidy unrelated code.
Best practice for non-trivial changes is to draft documentation and/or tests, get feedback on that, and then implement.
If you are assigned an issue, that means you are actively working on it or will do so in the near future. “I’ll get to this later” should not be assigned to you.
PR review:
- If you think you’re done and it’s ready to merge: Tag the PR
ready to merge
. Don’t request review from the project lead. - If you think you’re done and want review from someone other than the project lead: Request review from that person using the GitHub web interface. Once the PR passes review, go to the previous item.
- If you’re not done but want feedback: Request review from the person you want to review, which can be the project lead.
The purpose of this approach is to provide an easy way to see what PRs are ready to go, without the project lead needing to consult both the list of PRs and their own list of review requests, and also to provide a way to request reviews from the project lead without also requesting merge.
Comments should all be packaged up into a single review; click Start a review rather than Add single comment. Then the PR author gets only a single notification instead of one for every comment you make.
- If you think you’re done and it’s ready to merge: Tag the PR
Closing issues: We close issues when we’ve taken the requested action, decided not to take action, resolved the question, or actively determined an issue is obsolete. It is OK for “stale” issues to sit around indefinitely awaiting this. Unlike many projects, we do not automatically close issues just because they’re old.
Stale PRs, on the other hand, are to be avoided due to bit rot. We try to either merge or reject PRs in a timely manner.
Closed issues can be re-opened if new information arises, for example a
worksforme
issue with new reproduction steps. Please comment to ask for re-opening rather than doing it yourself.
7.1.3. GitHub issue and PR tags¶
7.1.3.1. What kind of issue is it?¶
bug
- Problem of some kind that needs to be fixed; i.e., something doesn’t work. This includes usability and documentation problems. Should have steps to reproduce with expected and actual behavior.
enhancement
- Things work, but it would be better if something was different. For example, a new feature proposal or refactoring. Should have steps to reproduce with desired and actual behavior.
help wanted
- The core team does not plan to address this issue, perhaps because we don’t know how, but we think it would be good to address it. We hope someone from the community will volunteer.
key issue
- A particularly important or notable issue.
question
- Support request that does not report a problem or ask for a change.
7.1.3.2. What do we plan to do about it?¶
For all of these, leave other tags in place, e.g. bug
.
deferred
- No plans to do this, but not rejected. These issues stay open, because we do not consider the deferred state resolved. Submitting PRs on these issues is risky; you probably want to argue successfully that it should be done before starting work on it.
duplicate
- Same as some other previously reported issue. In addition to this tag, duplicates should refer to the other issue and be closed.
obsolete
- No longer relevant, moot, etc. Close.
erroneous
- Not a Charliecloud issue; close. Use caution when blaming a problem on user error. Often (or usually) there is a documentation or usability bug that caused the “user error”.
ready to merge
- PRs only. Adding this tag states that the PR is complete and requests it be merged to master. If the project lead requests changes, they’ll remove the tag. Re-add it when you’re ready to try again. Lead removes tag after merging.
wontfix
- We are not going to do this, and we won’t merge PRs. Close issue after tagging, though sometimes you’ll want to leave a few days to allow for further discussion to catch mistaken tags.
worksforme
- We cannot reproduce the issue. Typical workflow is to tag, then wait a few days for clarification before closing.
7.1.4. Testing¶
PRs will not be merged until they pass the tests.
- Tests should pass on your development box as well as all relevant clusters, in full scope. (Note that some of the examples take quite a long time to build; the Docker cache is your friend.)
- All the Travis tests should pass. If you’re iterating trying to make Travis happy, consider interactive rebase, amending commits, or a throwaway branch. Don’t submit a PR with half a dozen “fix Travis” commits.
test/docker-clean.sh
can be used to purge your Docker cache, either by removing all tags or deleting all containers and images. The former is generally preferred, as it lets you update only those base images that have actually changed (the ones that haven’t will be re-tagged).
7.2. Documentation¶
7.2.1. How to build the documentation¶
This documentation is built using Sphinx with the sphinx-rtd-theme. It lives
in doc-src
.
7.2.1.1. Prerequisites¶
- Python 3.5+
- Sphinx 1.4.9+
- docutils 0.13.1+
- sphinx-rtd-theme 0.2.4+
Older versions may work but are untested.
7.2.1.2. To build the HTML¶
Install the prerequisites:
$ pip3 install sphinx sphinx-rtd-theme
Then:
$ cd doc-src
$ make
The HTML files are copied to doc
with rsync
. Anything to not
copy is listed in RSYNC_EXCLUDE
.
There is also a make clean
target that removes all the derived files
as well as everything in doc
.
Note
If you’re on Debian Stretch or some version of Ubuntu, this will silently
install into ~/.local
, leaving the sphinx-build
binary in
~/.local/bin
, which is often not on your path. One workaround
(untested) is to run pip3
as root, which violates principle of
least privilege. A better workaround, assuming you can write to
/usr/local
, is to add the undocumented and non-standard
--system
argument to install in /usr/local
instead. (This
matches previous pip
behavior.) See Debian bugs 725848 and 820856.
7.2.1.3. Publishing to the web¶
If you have write access to the repository, you can update the web documentation (i.e., http://hpc.github.io/charliecloud).
Normally, doc
is a normal directory ignored by Git. To publish to the
web, that diretory needs to contain a Git checkout of the gh-pages
branch (not a submodule). To set that up:
$ rm -Rf doc
$ git clone git@github.com:hpc/charliecloud.git doc
$ cd doc
$ git checkout gh-pages
To publish:
$ make web
It sometimes takes a few minutes for the web pages to update.
7.3. Coding style¶
We haven’t written down a comprehensive style guide. Generally, follow the style of the surrounding code, think in rectangles rather than lines of code or text, and avoid CamelCase.
Note that Reid is very picky about style, so don’t feel singled out if he complains (or even updates this section based on your patch!). He tries to be nice about it.
7.3.1. Writing English¶
When describing what something does (e.g., your PR or a command), use the imperative mood, i.e., write the orders you are giving rather than describe what the thing does. For example, do:
Inject files from the host into an image directory.Add--join-pid
option toch-run
.Do not (indicative mood):
Injects files from the host into an image directory.Adds--join-pid
option toch-run
.Use sentence case for titles, not title case.
If it’s not a sentence, start with a lower-case character.
Use spell check. Keep your personal dictionary updated so your editor is not filled with false positives.
7.3.2. curl
vs. wget
¶
For URL downloading in shell code, including Dockerfiles, use wget -nv
.
Both work fine for our purposes, and we need to use one or the other
consistently. According to Debian’s popularity contest, 99.88% of reporting
systems have wget
installed, vs. about 44% for curl
. On the
other hand, curl
is in the minimal install of CentOS 7 while
wget
is not.
For now, Reid just picked wget
because he likes it better.
7.3.3. Variable conventions in shell scripts and .bats
files¶
Separate words with underscores.
User-configured environment variables: all uppercase,
CH_TEST_
prefix. Do not use in individual.bats
files; instead, provide an intermediate variable.Variables local to a given file: lower case, no prefix.
Bats: set in
common.bash
and then used in.bats
files: lower case,ch_
prefix.Surround lower-case variables expanded in strings with curly braces, unless they’re the only thing in the string. E.g.:
"${foo}/bar" # yes "$foo" # yes "$foo/bar" # no "${foo}" # no
Quote the entire string instead of just the variable when practical:
"${foo}/bar" # yes "${foo}"/bar # no "$foo"/bar # no