canmove, Confirmed users
1,394
edits
ReleaseEngineering/PuppetAgain/HowTo/Hack on PuppetAgain: Difference between revisions
Jump to navigation
Jump to search
ReleaseEngineering/PuppetAgain/HowTo/Hack on PuppetAgain (view source)
Revision as of 23:25, 10 July 2012
, 10 July 2012no edit summary
(Created page with "= Hacking = PuppetAgain is (or, at least, will be) a [http://www.mozilla.org/hacking/module-ownership.html code module], and as such people working on PuppetAgain follow the usua...") |
No edit summary |
||
| Line 8: | Line 8: | ||
The last point is particularly important, since puppet masters automatically update to the latest commit, and start deploying that to hosts. | The last point is particularly important, since puppet masters automatically update to the latest commit, and start deploying that to hosts. | ||
= Patch Guidelines = | = Patch Guidelines / Review Checklist = | ||
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. | |||
== 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) | * SSH authorized_keys (note that known_hosts don't hurt) | ||
* usernames (cltbld isn't universal) | * usernames (cltbld isn't universal) | ||
* hostnames | * 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. | 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: | Keep code where it belongs: | ||
* Node declarations should ''only'' set variables and include toplevel classes (this is looking forward to using an ENC). | * Node declarations should ''only'' set variables and include toplevel classes (this is looking forward to using an ENC). | ||
| Line 21: | Line 25: | ||
* 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. | * 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. | * 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 [[ReleaseEngineering/PuppetAgain/Modules/packages|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 [[ReleaseEngineering/PuppetAgain/Repositories|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. | 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 <tt>toplevel::*</tt> 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. | |||