Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion _docs/reviewing_pr.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ one of our modules:
* Is a new class added? It should have unit tests using [rpsec-puppet-facts](https://github.com/mcanevet/rspec-puppet-facts#rspec-puppet-facts) that at least verify that the new class compiles. It also needs to have [puppet-strings docs](https://puppet.com/docs/puppet/6.17/puppet_strings.html).
* Files should always terminate with a newline if possible, with an exception being file or template fragments like those used with concat. This is the [POSIX standard][posix], and some tools don't handle the lack of a terminating newline properly
* If you can supply one or multiple values for an attribute it's common practice to enforce the datatype for one value and an array of that datatype. An example for string is `Variant[String[1],Array[String[1]]]`. This can be used in the Puppet code as `[$var].flatten()`
* The parameter section should always be aligned at the `=` char
* In the past, the parameter section of a class or defined resource was always aligned at the `=` char. Sometimes there was an additional alignment at the `$` from the parameter name. Reordering this when a new parameter is introduced or when a datatype is changed, causes a lot of noise. Also it makes rebases, reverts and cherry-picks quite hard. New parameters should only have a single whitespace between the datatype and the variable, and also only a single whitespace for the default value. Example: `String[1] $foo = 'foo',`.
* Is a class considered private? Then it should contain [assert_private][as]
* A module should have as few public interfaces as possible. It should be aimed for the init.pp being the only public class. This is not a rule but a general guideline. Depending on the module, it is not always possible or feasible to configure everything through a single class.
* Is another module added as a dependency? Add it to the `.fixtures.yml` file as a git repository (as a `https://` link, not `ssh` or `git://`). Spec tests always run against master branches to detect breaking changes as early as possible. Acceptance tests use the last release (installed by [install_module_dependencies][imd] which parses it from the `metadata.json`)
Expand Down