ReleaseEngineering/PuppetAgain/HowTo/Hack on PuppetAgain

Hacking

PuppetAgain is (or, at least, will be) a code module, and as such people working on PuppetAgain follow the usual Mozilla process for making changes:

• hack
• post a patch, request review from a module owner or peer
• repeat until r+ (and sr+ if necessary)
• commit
• monitor and handle any problems as a result of the commit

The last point is particularly important, since puppet masters automatically update to the latest commit, and start deploying that to hosts.

Patch Guidelines / Review Checklist

Be Generic

PuppetAgain should be usable externally. Do not hard-code things that external users may want to adjust, and extract them from CSV so they can be overridden easily. That includes:

• SSH authorized_keys (note that known_hosts don't hurt)
• usernames (cltbld isn't universal)
• hostnames

Changes Only

Do not write resources that will be instantiated on every puppet run. This makes it difficult to tell if a particular puppet run has actually done anything, and will result in misleading data in the puppet dashboard. Note that the "unless" or "onlyif" parameters to Exec, while they still run an external command (and thus aren't especially efficient), can effectively prevent a command from running.

Be Modular

Keep code where it belongs:

• Node declarations should only set variables and include toplevel classes (this is looking forward to using an ENC).
• Toplevel classes should only include other classes, although parameterized classes are OK. Don't do anything substantial directly in a toplevel class -- make a new module and include it.
• Package classes should only install packages. These classes should be extremely formulaic, and readers should not need to look at them to figure out what they do. Conversely, nothing else should install packages.
• Include requirements near where they are needed. It never hurts to include packages::foo from several places, if foo is used in several places.
• Many modules have a few variables that are needed throughout the module. In this case, make a modulename::settings class that contains only these variables, then include it in all of the classes where those variables are needed.
• In general, more, smaller, more easily-digested classes are better than single, large classes.

DRY

(Don't Repeat Yourself) This applies in a few cases:

• If your action module has a long list of repetitive resources (file, exec, etc.), it can probably be refactored nicely into a utility class or define. This applies even for long lists of file resources. If that utility class is not purpose-specific, put it in its own module so it can be shared.
• When customizing a configuration file for different purposes, it's tempting to just use several files and source => "puppet:///modules/${module_name}/myfile.${purpose}". In many cases those files share common code, which will be difficult to keep in sync as it evolves. In this case, prefer to use a template or concatenation to construct the configuration file with the common parts only specified once.

Use the Toplevel Naming Structure

The toplevel naming hierarchy and inheritance hierarchy should be identical. That is, toplevel::foo::bar::baz should be a subclass of toplevel::foo::bar, which is a subclass of toplevel::foo, which is a subclass of toplevel.

Common Directories

We tend to use the same directories for multiple projects. That's what the dirs module is for! Creating extra dirs classes is easy and never hurts.

Packages

Remember that we're running puppet on multiple operating systems. If a new package is only required on one operating system, include a case $operatingsystem and call fail() in the default branch, so that it will fail loudly if the class is applied on another operating system. If a utility is installed by default on some operating systems, include a clause in the case with a comment indicating that.

Custom Packages

If you create a custom package, it should be installed with a class named python::mozilla::$package, so it's clear that it's customized. The package should be documented on the packages module page.

For RPMs, the spec file should be checked into build/puppet in modules/packages/manifests/mozilla, with the same base name as the .pp file. When you land the changes, the resulting RPM and SRPM should both be added to the appropriate repository. The contents (but maybe not filename) of the spec file embedded in the SRPM should match the checked-in spec file exactly. This system allows (a) review of spec changes and (b) easy rebuilding of custom RPMS. See ReleaseEngineering/PuppetAgain/HowTo/Modify a Custom RPM for hints on how to build RPMs.

Strong Dependencies

Wherever a dependency might exist, specify it. Dependency errors are difficult to spot when you're incrementally building a system as you develop your patch. Unspecified dependencies can even work in production -- for a while, until a new and unrelated resource shakes up the ordering and suddenly things don't work.

You can get puppet to give you a dependency graph in .dot format! See http://bitfieldconsulting.com/puppet-dependency-graphs

In general, use require => Class['some::class'] rather than requiring a basic resource that you know the class defines (especially avoid require => Package['some-package']). This way, you are not depending on an implementation detail of that other class. This also creates many more dependencies (on all of the resources in that class), which keeps things predictable.

Keep in mind that puppet automatically requires parent directories, so File['/builds/slave'] automatically requires File['/builds'] if it exists. Also, note that in many cases packages will write default configuration files when they are installed, so you should make sure that the File resource installing the configuration requires the package class.

Document

Adjust/amend the documentation on the wiki while landing the change after review.

No Bad Habits

• Even if all of the FooAPI servers are running OpenSolaris, $::operatingsystem=="Solaris" does not mean this is a FooAPI server. OS does not imply role. Roles are defined in node declarations, by including toplevel::* classes.
• It can be tempting to put "just one resource" into an already-existing class. User::builder seems to be a popular target. Adding new classes is easy, and avoids surprises. If the resource you're adding isn't obviously tied to the name of the class you're adding it to, put it in another (probably new) class.

Config Files

• For file-resources where you can easily implement a run-parts format design, do so, rather than modify the global config.
  • Examples crontab vs cron.d/*, http.d.
• You should specify the parent dir to purge and recursive, so that when you remove a resource from puppet it removes itself from the machine.
• The crontab resource has complex gotchyas, *always* use cron.d/* for cron.
• config resources which are run automatically should explicitly depend on the Classes they need to run properly.

Nit-Worthy Code Style

While everyone will have different opinions on the best Coding Style, one thing we can all agree on, life is better when we all agree. So the following are Nit's we should all strive to adhere to, but exceptions can be made with good reasoning. (mistakes can sneak in, if so, just edit them out next time you touch that area of code)

Whitespace and Tabs

• TABS: NEVER in a puppet manifest file. ONLY when necessary in other files.
• Trailing Whitespace: Shouldn't exist.
• EOL: Always Unix style line-endings.
• EOF: Always include a newline at EOF
• Indentation: Four Space indents.

Comments

• We prefer the # style comments throughout, C-Style is frowned upon.
• Manifests that are not very obvious by their name should have a very brief comment at the top of the file describing it, these comments are informative, normative reference is the wiki docs.
• When it helps understanding of why something is in the manifest (or not in the manifest) please include brief comment saying so.