Issue 1281 - Introduce module buildbot

modules/buildbot/manifests/init.pp

Index: modules/buildbot/manifests/init.pp
diff --git a/modules/buildbot/manifests/init.pp b/modules/buildbot/manifests/init.pp
new file mode 100644
index 0000000000000000000000000000000000000000..1e8b43cfbe649ce05714fe7f618ea60b5451f68f
--- /dev/null
+++ b/modules/buildbot/manifests/init.pp
@@ -0,0 +1,151 @@
+# == Class: buildbot
+#
+# Manage Buildbot (https://buildbot.net/) master and slave setups.
+#
+# Class buildbot acts as the root namespace for the buildbot integration
+# with Puppet, while also providing a variety of setup parameters that can
+# be used to adjust the software setup.
+#
+# It defines a set of virtual resources titled 'buildmaster' and 'buildslave'.
+# Those are realized implicitly when required by any of the various entities
+# within the module, but may become realized explicitely when necessary:
+#
+# - Concat[]: The 'buildmaster' and 'buildslave' system daemon configuration
+# - Concat::Fragment[]: The respective root or configuration head fragment
+# - File[]: The anchestor of default for master and slave $basedir resources
+# - Service[]: The actual services running the master and slave instances
+#
+# Note, however, that the respective instances are rather implementation
+# specific and thus may become subject to change in the future.
+#
+# === Parameters:
+#
+# [*master_config*]
+#   The path to the 'buildmaster' system configuration.
+#
+# [*master_directory*]
+#   The anchestor of the default buildbot::master $basedir.

[Felix Dahlke, 2015/10/02 12:16:26]

Typo.

[mathias, 2015/10/02 13:47:40]

Acknowledged.

+#
+# [*master_pacakges*]

[Felix Dahlke, 2015/10/02 12:16:26]

Typo.

[mathias, 2015/10/02 13:47:40]

Acknowledged.

+#   The packages that provide the buildbot::master dependencies.
+#
+# [*master_runner*]
+#   The path to the runner used for buildbot::master instances.
+#
+# [*master_service*]
+#   The status 'buildmaster' service status to ensure, if any.

[Felix Dahlke, 2015/10/02 12:16:26]

One "status" should suffice.

[mathias, 2015/10/02 13:47:40]

Acknowledged.

+#
+# [*master_user*]
+#   The user operating the 'buildmaster' service.
+#
+# [*slave_config*]
+#   The path to the 'buildslave' system configuration.
+#
+# [*slave_directory*]
+#   The anchestor of the default buildbot::slave $basedir.
+#
+# [*slave_pacakges*]
+#   The packages that provide the buildbot::slave dependencies.
+#
+# [*slave_runner*]
+#   The path to the runner used for buildbot::slave instances.
+#
+# [*slave_service*]
+#   The status 'buildslave' service status to ensure, if any.
+#
+# [*slave_user*]
+#   The user operating the 'buildslave' service.
+#
+# === Examples:
+#
+#   class {'buildbot':
+#     master => 'running',
+#   }
+#
+class buildbot (

[Felix Dahlke, 2015/10/02 12:16:26]

For some reason, I haven't yet found the part where buildbot is actually being
installed. But assuming there are separate packages for master and slave - why
configure them together like this? In practice, a node should be either a master
or a slave, not both.

If it's one package for both, and it's being installed here somehow, nevermind.

[mathias, 2015/10/02 13:47:40]

Types buildbot::master and buildbot::slave will
ensure_packages($::buildbot::{master,slave}_packages) when necessary. In
addition, when either ${master,slave}_service is defined,
ensure_packages(${master,slave}_packages) is invoked by class buildbot (see the
bottom of this file).
Note that all resources contained by class buildbot are virtual. Entities within
the buildbot namespace realize those when necessary. The options here do not
only configure these virtual resources, but are also re-used by the other
entities.

I prefer this method over the more common approach where one would have an
almost empty buildbot class, because..

1. If using internal variables (e.g. to inherit from params, and then use $var
directly), it's less transparent, confusing authors about where what has been
defined, and often feature a misleading name ($foo::params).

2. If using actual parameters (e.g. $foo::params::var), it's quite repetetive
without any gain over the inclusion here whatsoever.

3. Package specific decisions, ordering and quirks would be scattered over two
different files, at least. In cnotrast, all resources in class buildbot address
package-specific aspects, not the actual buildbot master or slave configuration,
which are done in types ::master and ::slave.

4. Using the root class instead of params avoids common Puppet bugs with
inclusion order, e.g. https://projects.puppetlabs.com/issues/11875 (that
particular one was the reason for multiple issues in our tracker before, for
both our modules and 3rd party ones), whilst being agnostic to the Puppet
version in use (2 or 3).

[Felix Dahlke, 2015/10/02 15:44:27]

Oh, now I get it, the packages to use and some details about them are
configurable... To be honest, I'm not happy with the complexity this adds. It
makes things harder to understand (at least for me), it's inconsistent with how
we do things in the other modules, and I don't actually see a use case for it.

If I was using this class, I would have used it wrong and spent some time
debugging. To me, $slave_packages seemed to be about packages I need in order to
compile my stuff, such as `build-essentials`, so I would have overwritten this.

I don't fully understand what problems this approach solves, but since
consistency is most important, we should do it the same way we do it in our
other modules here, and continue this discussion elsewhere.

[mathias, 2015/10/02 16:25:53]

Providing those options is a trait of this approach, not the reason why it has
been chosen.
Actually it reduces complexity, but I agree that this is not as trivial to
understand as the more common approach mentioned before. 
 
The documentation of types buildbot::master and buidlbot::slave is quite verbose
in my opinion. So is that really an issue, or maybe attributed to splitting up
the review in multiple sessions? Maybe it would be enough to extend the
documentation of class buildbot by actual examples including the other types;
one who would "just use" the module in the future may understand it better then.
 
While consistency is of course an important aspect, there is also reason to
evolve in "how we do things", especially when and where more sophisticated
solutions are required (the Puppet bug mentioned before, for example). The
approach here could serve as an example for future modules and, if proven to
actually be better, become the new "how we do things".

Note also that most of the existing modules neither comply with the officially
chosen coding and style guides, nor employ any advanced techniques in general.

Anyway, I do read your notion of this being not quite optimal yet. Let's see if
I can combine both sides concerns into a single approach, one that should also
ease insight in the concept of the applied pattern.

[Felix Dahlke, 2015/10/02 16:48:48]

Please note that I'm fine with what the buildbot class here does, how I see it
it reduces duplication.

I'm solely questioning why $*_config etc. are parameters, rather than variables.
If there is no other good way to share these constants between buildbot,
buildbot::master and buildbot::slave, I guess it's fine...

Surely, improving how we do things is important. But I'd prefer to do that by
first agreeing on the approach, ensuring it is reflected in our coding style, if
necessary, and having a plan for enforcing it to the old code over time.

[mathias, 2015/10/20 20:36:16]

Acknowledged.

+  $master_config = '/etc/default/buildmaster',
+  $master_directory = '/var/buildmaster',
+  $master_packages = ['buildbot'],
+  $master_runner = '/usr/bin/buildbot',
+  $master_service = undef,
+  $master_user = 'buildbot',
+  $slave_config = '/etc/default/buildslave',
+  $slave_directory = '/var/buildslave',
+  $slave_packages = ['buildbot-slave'],
+  $slave_runner = '/usr/bin/buildslave',
+  $slave_service = undef,
+  $slave_user = 'buildbot',
+) {
+
+  @concat {
+    'buildmaster':
+      owner => $master_user,
+      path => $master_config,
+      require => Package[$master_packages];
+    'buildslave':
+      owner => $slave_user,
+      path => $slave_config,
+      require => Package[$slave_packages];
+  }
+
+  @concat::fragment {
+    'buildmaster':
+      content => template('buildbot/buildmaster.erb'),
+      order => 0,
+      target => 'buildmaster';
+    'buildslave':
+      content => template('buildbot/buildslave.erb'),
+      order => 0,
+      target => 'buildslave';
+  }
+
+  @file {
+    $master_directory:
+      ensure => 'directory',
+      owner => $master_user,
+      require => Package[$master_packages];
+    $slave_directory:
+      ensure => 'directory',
+      owner => $slave_user,
+      require => Package[$slave_packages];
+  }
+
+  Service {
+    hasrestart => true,
+    hasstatus => false,
+  }
+
+  @service {
+    'buildmaster':
+      ensure => $master_service ? {
+        /^(running|started|true)$/ => 'running',
+        default => 'stopped',
+      },
+      pattern => "^$master_user.*python.*$master_runner",
+      require => Package[$master_packages];
+    'buildslave':
+      ensure => $slave_service ? {
+        /^(running|started|true)$/ => 'running',
+        default => 'stopped',
+      },
+      pattern => "^$slave_user.*python.*$slave_runner",
+      require => Package[$slave_packages];
+  }
+
+  Service['buildmaster'] <~ Exec <|creates == $master_config|>
+  Service['buildmaster'] <~ File <|path == $master_config|>
+  Service['buildmaster'] ~>
+
+  Service['buildslave'] <~ Exec <|creates == $slave_config|>
+  Service['buildslave'] <~ File <|path == $slave_config|>
+
+  if $master_service != undef {
+    ensure_packages($master_packages)
+    realize(Service['buildmaster'])
+  }
+
+  if $slave_service != undef {
+    ensure_packages($slave_packages)
+    realize(Service['buildslave'])
+  }
+}