#2317 - provide role fileserver for eyeofiles.com

modules/adblockplus/manifests/web/fileserver.pp

 # == Class: adblockplus::web::fileserver
 #
-# Serves files for different repositories over https.
+# A fileserver serves multiple file repositories.
 #
 # === Parameters:
 #
 # [*domain*]
-#   A string for the domain serving traffic.

[mathias, 2017/09/22 07:00:14]

Why would the domain want to have some string? And how does a domain serve
traffic anyway? Isn't this more the name of the domain associated with the
service, given as a string?

[f.nicolaisen, 2017/09/22 07:57:44]

Acknowledged.

+#   A string which is the name of the fileserver domain, under which
+#   each repository has a subdomain.
 #
 # [*certificate*]
 #   The name of the SSL certificate file within modules/private/files, if any.
 #   Requires a private_key as well.
 #
 # [*private_key*]
 #   The name of the private key file within modules/private/files, if any.
 #   Requires a certificate as well.
 #
 # [*is_default*]

[mathias, 2017/09/22 07:00:14]

The fileserver is a standalone HTTPd setup intended for use with a
wildcard-cert, hence I'd say $nginx::hostconfig::is_default should always be
true (hard-coded inside this class here, not a parameter from the outside).

[f.nicolaisen, 2017/09/22 07:57:44]

Acknowledged.

 #  Passed on to nginx (whether or not the site config should be default).
 #
-# [*repository*]

[mathias, 2017/09/22 07:00:14]

The documentation of the $repository parameter does not fit anymore.

[f.nicolaisen, 2017/09/22 07:57:43]

Acknowledged.

-#   A string that is the name of the repository to serve. A repository
-#   is a collection of files uploaded by a group of users.
-#
-#   TODO Convert this into a collection of *repositories*
+# [*repositories*]
+#   A collection (hash) of repositories to serve.
+#   The contents of a repository is served on a subdomain of the fileserver.
 #
 class adblockplus::web::fileserver(
+  $ensure = 'present',
   $domain,
-  $certificate,

[mathias, 2017/09/22 07:00:14]

Both $certificate and $private_key should default to undef, which allows to not
use a self-signed certificate in development but plain HTTP, which is what we do
in new setups since a few years. The idea is to not introduce complexity in
development, so one does not have fun clicking away pointless browser messages,
does not need hacking exceptions into adjacent modules and scripts (i.e.
--no-check-certificate), does not need to maintain development CAs, and does not
learn how to ignore security warnings.

[f.nicolaisen, 2017/09/22 07:57:43]

Acknowledged.

-  $private_key,
-  $is_default=false,
+  $certificate = undef,
+  $private_key = undef,
   $repositories={},
 ){
 
   include nginx
   include adblockplus
   include adblockplus::web
 
-  nginx::hostconfig{$domain:
-    source => 'puppet:///modules/adblockplus/nginx/fileserver.conf',
-    is_default => $is_default,
-    certificate => $certificate,
-    private_key => $private_key,
-    log => 'access_log_files'
-  }
-
-  # Root directory for serving repositories
   realize(File[$adblockplus::directory])
 
-
-  file {[
-    "$adblockplus::directory/fileserver",
-    "$adblockplus::directory/fileserver/repositories"
-  ]:
+  file {"$adblockplus::directory/fileserver":
     ensure => directory,
   }
 
+  ensure_resources('adblockplus::web::fileserver::repository', $repositories, {
+    ensure => 'present',
+  })
 
-  if !empty($repositories) {

[mathias, 2017/09/22 07:00:14]

This conditional is not necessary: Iterating through an empty $repositories hash
(the default) won't do anything. You of course need to remove the pointless
assignment of undef/null in the associated role.yaml file.

[f.nicolaisen, 2017/09/22 07:57:44]

Acknowledged.

-    ensure_resources('repository', $repositories, {

[mathias, 2017/09/22 07:00:14]

Please use the fully qualified name here, e.g. adblockplus::web::repository.

[f.nicolaisen, 2017/09/22 07:57:43]

Acknowledged.

-      ensure => 'present',
-    })
+  nginx::hostconfig{ "$domain":
+    source => 'puppet:///modules/adblockplus/nginx/fileserver.conf',
+    is_default => true,
+    certificate => $certificate,
+    private_key => $private_key,
+    log => 'access_log_fileserver',
   }
-
 }