|
| 1 | +# Contributing to Puppet modules |
| 2 | + |
| 3 | +So you want to contribute to a Puppet module: Great! Below are some instructions to get you started doing |
| 4 | +that very thing while setting expectations around code quality as well as a few tips for making the |
| 5 | +process as easy as possible. |
| 6 | + |
| 7 | +### Table of Contents |
| 8 | + |
| 9 | +1. [Getting Started](#getting-started) |
| 10 | +1. [Commit Checklist](#commit-checklist) |
| 11 | +1. [Submission](#submission) |
| 12 | +1. [More about commits](#more-about-commits) |
| 13 | +1. [Testing](#testing) |
| 14 | + - [Running Tests](#running-tests) |
| 15 | + - [Writing Tests](#writing-tests) |
| 16 | +1. [Get Help](#get-help) |
| 17 | + |
| 18 | +## Getting Started |
| 19 | + |
| 20 | +- Fork the module repository on GitHub and clone to your workspace |
| 21 | + |
| 22 | +- Make your changes! |
| 23 | + |
| 24 | +## Commit Checklist |
| 25 | + |
| 26 | +### The Basics |
| 27 | + |
| 28 | +- [x] my commit is a single logical unit of work |
| 29 | + |
| 30 | +- [x] I have checked for unnecessary whitespace with "git diff --check" |
| 31 | + |
| 32 | +- [x] my commit does not include commented out code or unneeded files |
| 33 | + |
| 34 | +### The Content |
| 35 | + |
| 36 | +- [x] my commit includes tests for the bug I fixed or feature I added |
| 37 | + |
| 38 | +- [x] my commit includes appropriate documentation changes if it is introducing a new feature or changing existing functionality |
| 39 | + |
| 40 | +- [x] my code passes existing test suites |
| 41 | + |
| 42 | +### The Commit Message |
| 43 | + |
| 44 | +- [x] the first line of my commit message includes: |
| 45 | + |
| 46 | + - [x] an issue number (if applicable), e.g. "(MODULES-xxxx) This is the first line" |
| 47 | + |
| 48 | + - [x] a short description (50 characters is the soft limit, excluding ticket number(s)) |
| 49 | + |
| 50 | +- [x] the body of my commit message: |
| 51 | + |
| 52 | + - [x] is meaningful |
| 53 | + |
| 54 | + - [x] uses the imperative, present tense: "change", not "changed" or "changes" |
| 55 | + |
| 56 | + - [x] includes motivation for the change, and contrasts its implementation with the previous behavior |
| 57 | + |
| 58 | +## Submission |
| 59 | + |
| 60 | +### Pre-requisites |
| 61 | + |
| 62 | +- Make sure you have a [GitHub account](https://github.com/join) |
| 63 | + |
| 64 | +- [Create a ticket](https://tickets.puppet.com/secure/CreateIssue!default.jspa), or [watch the ticket](https://tickets.puppet.com/browse/) you are patching for. |
| 65 | + |
| 66 | +### Push and PR |
| 67 | + |
| 68 | +- Push your changes to your fork |
| 69 | + |
| 70 | +- [Open a Pull Request](https://help.github.com/articles/creating-a-pull-request-from-a-fork/) against the repository in the puppetlabs organization |
| 71 | + |
| 72 | +## More about commits |
| 73 | + |
| 74 | + 1. Make separate commits for logically separate changes. |
| 75 | + |
| 76 | + Please break your commits down into logically consistent units |
| 77 | + which include new or changed tests relevant to the rest of the |
| 78 | + change. The goal of doing this is to make the diff easier to |
| 79 | + read for whoever is reviewing your code. In general, the easier |
| 80 | + your diff is to read, the more likely someone will be happy to |
| 81 | + review it and get it into the code base. |
| 82 | + |
| 83 | + If you are going to refactor a piece of code, please do so as a |
| 84 | + separate commit from your feature or bug fix changes. |
| 85 | + |
| 86 | + We also really appreciate changes that include tests to make |
| 87 | + sure the bug is not re-introduced, and that the feature is not |
| 88 | + accidentally broken. |
| 89 | + |
| 90 | + Describe the technical detail of the change(s). If your |
| 91 | + description starts to get too long, that is a good sign that you |
| 92 | + probably need to split up your commit into more finely grained |
| 93 | + pieces. |
| 94 | + |
| 95 | + Commits which plainly describe the things which help |
| 96 | + reviewers check the patch and future developers understand the |
| 97 | + code are much more likely to be merged in with a minimum of |
| 98 | + bike-shedding or requested changes. Ideally, the commit message |
| 99 | + would include information, and be in a form suitable for |
| 100 | + inclusion in the release notes for the version of Puppet that |
| 101 | + includes them. |
| 102 | + |
| 103 | + Please also check that you are not introducing any trailing |
| 104 | + whitespace or other "whitespace errors". You can do this by |
| 105 | + running "git diff --check" on your changes before you commit. |
| 106 | + |
| 107 | + 2. Sending your patches |
| 108 | + |
| 109 | + To submit your changes via a GitHub pull request, we _highly_ |
| 110 | + recommend that you have them on a topic branch, instead of |
| 111 | + directly on "master". |
| 112 | + It makes things much easier to keep track of, especially if |
| 113 | + you decide to work on another thing before your first change |
| 114 | + is merged in. |
| 115 | + |
| 116 | + GitHub has some pretty good |
| 117 | + [general documentation](http://help.github.com/) on using |
| 118 | + their site. They also have documentation on |
| 119 | + [creating pull requests](https://help.github.com/articles/creating-a-pull-request-from-a-fork/). |
| 120 | + |
| 121 | + In general, after pushing your topic branch up to your |
| 122 | + repository on GitHub, you can switch to the branch in the |
| 123 | + GitHub UI and click "Pull Request" towards the top of the page |
| 124 | + in order to open a pull request. |
| 125 | + |
| 126 | + 3. Update the related JIRA issue. |
| 127 | + |
| 128 | + If there is a JIRA issue associated with the change you |
| 129 | + submitted, then you should update the ticket to include the |
| 130 | + location of your branch, along with any other commentary you |
| 131 | + may wish to make. |
| 132 | + |
| 133 | +# Testing |
| 134 | + |
| 135 | +## Getting Started |
| 136 | + |
| 137 | +Our Puppet modules provide [`Gemfile`](./Gemfile)s, which can tell a Ruby package manager such as [bundler](http://bundler.io/) what Ruby packages, |
| 138 | +or Gems, are required to build, develop, and test this software. |
| 139 | + |
| 140 | +Please make sure you have [bundler installed](http://bundler.io/#getting-started) on your system, and then use it to |
| 141 | +install all dependencies needed for this project in the project root by running |
| 142 | + |
| 143 | +```shell |
| 144 | +% bundle install --path .bundle/gems |
| 145 | +Fetching gem metadata from https://rubygems.org/........ |
| 146 | +Fetching gem metadata from https://rubygems.org/.. |
| 147 | +Using rake (10.1.0) |
| 148 | +Using builder (3.2.2) |
| 149 | +-- 8><-- many more --><8 -- |
| 150 | +Using rspec-system-puppet (2.2.0) |
| 151 | +Using serverspec (0.6.3) |
| 152 | +Using rspec-system-serverspec (1.0.0) |
| 153 | +Using bundler (1.3.5) |
| 154 | +Your bundle is complete! |
| 155 | +Use `bundle show [gemname]` to see where a bundled gem is installed. |
| 156 | +``` |
| 157 | + |
| 158 | +NOTE: some systems may require you to run this command with sudo. |
| 159 | + |
| 160 | +If you already have those gems installed, make sure they are up-to-date: |
| 161 | + |
| 162 | +```shell |
| 163 | +% bundle update |
| 164 | +``` |
| 165 | + |
| 166 | +## Running Tests |
| 167 | + |
| 168 | +With all dependencies in place and up-to-date, run the tests: |
| 169 | + |
| 170 | +### Unit Tests |
| 171 | + |
| 172 | +```shell |
| 173 | +% bundle exec rake spec |
| 174 | +``` |
| 175 | + |
| 176 | +This executes all the [rspec tests](http://rspec-puppet.com/) in the directories defined [here](https://github.com/puppetlabs/puppetlabs_spec_helper/blob/699d9fbca1d2489bff1736bb254bb7b7edb32c74/lib/puppetlabs_spec_helper/rake_tasks.rb#L17) and so on. |
| 177 | +rspec tests may have the same kind of dependencies as the module they are testing. Although the module defines these dependencies in its [metadata.json](./metadata.json), |
| 178 | +rspec tests define them in [.fixtures.yml](./fixtures.yml). |
| 179 | + |
| 180 | +### Acceptance Tests |
| 181 | + |
| 182 | +Some Puppet modules also come with acceptance tests, which use [beaker][]. These tests spin up a virtual machine under |
| 183 | +[VirtualBox](https://www.virtualbox.org/), controlled with [Vagrant](http://www.vagrantup.com/), to simulate scripted test |
| 184 | +scenarios. In order to run these, you need both Virtualbox and Vagrant installed on your system. |
| 185 | + |
| 186 | +Run the tests by issuing the following command |
| 187 | + |
| 188 | +```shell |
| 189 | +% bundle exec rake spec_clean |
| 190 | +% bundle exec rspec spec/acceptance |
| 191 | +``` |
| 192 | + |
| 193 | +This will now download a pre-fabricated image configured in the [default node-set](./spec/acceptance/nodesets/default.yml), |
| 194 | +install Puppet, copy this module, and install its dependencies per [spec/spec_helper_acceptance.rb](./spec/spec_helper_acceptance.rb) |
| 195 | +and then run all the tests under [spec/acceptance](./spec/acceptance). |
| 196 | + |
| 197 | +## Writing Tests |
| 198 | + |
| 199 | +### Unit Tests |
| 200 | + |
| 201 | +When writing unit tests for Puppet, [rspec-puppet][] is your best friend. It provides tons of helper methods for testing your manifests against a |
| 202 | +catalog (e.g. contain_file, contain_package, with_params, etc). It would be ridiculous to try and top rspec-puppet's [documentation][rspec-puppet_docs] |
| 203 | +but here's a tiny sample: |
| 204 | + |
| 205 | +Sample manifest: |
| 206 | + |
| 207 | +```puppet |
| 208 | +file { "a test file": |
| 209 | + ensure => present, |
| 210 | + path => "/etc/sample", |
| 211 | +} |
| 212 | +``` |
| 213 | + |
| 214 | +Sample test: |
| 215 | + |
| 216 | +```ruby |
| 217 | +it 'does a thing' do |
| 218 | + expect(subject).to contain_file("a test file").with({:path => "/etc/sample"}) |
| 219 | +end |
| 220 | +``` |
| 221 | + |
| 222 | +### Acceptance Tests |
| 223 | + |
| 224 | +Writing acceptance tests for Puppet involves [beaker][] and its cousin [beaker-rspec][]. A common pattern for acceptance tests is to create a test manifest, apply it |
| 225 | +twice to check for idempotency or errors, then run expectations. |
| 226 | + |
| 227 | +```ruby |
| 228 | +it 'does an end-to-end thing' do |
| 229 | + pp = <<-EOF |
| 230 | + file { 'a test file': |
| 231 | + ensure => present, |
| 232 | + path => "/etc/sample", |
| 233 | + content => "test string", |
| 234 | + } |
| 235 | + |
| 236 | + apply_manifest(pp, :catch_failures => true) |
| 237 | + apply_manifest(pp, :catch_changes => true) |
| 238 | + |
| 239 | +end |
| 240 | +
|
| 241 | +describe file("/etc/sample") do |
| 242 | + it { is_expected.to contain "test string" } |
| 243 | +end |
| 244 | +
|
| 245 | +``` |
| 246 | +
|
| 247 | +# If you have commit access to the repository |
| 248 | +
|
| 249 | +Even if you have commit access to the repository, you still need to go through the process above, and have someone else review and merge |
| 250 | +in your changes. The rule is that **all changes must be reviewed by a project developer that did not write the code to ensure that |
| 251 | +all changes go through a code review process.** |
| 252 | +
|
| 253 | +The record of someone performing the merge is the record that they performed the code review. Again, this should be someone other than the author of the topic branch. |
| 254 | +
|
| 255 | +# Get Help |
| 256 | +
|
| 257 | +### On the web |
| 258 | +* [Puppet help messageboard](http://puppet.com/community/get-help) |
| 259 | +* [Writing tests](https://docs.puppet.com/guides/module_guides/bgtm.html#step-three-module-testing) |
| 260 | +* [General GitHub documentation](http://help.github.com/) |
| 261 | +* [GitHub pull request documentation](http://help.github.com/send-pull-requests/) |
| 262 | +
|
| 263 | +### On chat |
| 264 | +* Slack (slack.puppet.com) #forge-modules, #puppet-dev, #windows, #voxpupuli |
| 265 | +* IRC (freenode) #puppet-dev, #voxpupuli |
| 266 | +
|
| 267 | +
|
| 268 | +[rspec-puppet]: http://rspec-puppet.com/ |
| 269 | +[rspec-puppet_docs]: http://rspec-puppet.com/documentation/ |
| 270 | +[beaker]: https://github.com/puppetlabs/beaker |
| 271 | +[beaker-rspec]: https://github.com/puppetlabs/beaker-rspec |
0 commit comments