#7320 - Introduce helpcenter role

modules/adblockplus/manifests/web/static.pp

 # == Class: adblockplus::web::static
 #
 # Manage a simple Nginx-based webserver for static content
 # that uses a customizable deployment script to e.g. fetch the content
 # from a repository server (ref. http://hub.eyeo.com/issues/4523)
-#

[mathias, 2018/04/05 01:12:30]

There's one empty line too many here.

[f.lopez, 2018/04/10 02:45:39]

Acknowledged.

 #
 # === Parameters:
 #
 # [*domain*]
 #   The domain name for the website.
 #
 # [*ssl_certificate*]
 #   The name of the SSL certificate file within modules/private/files, if any.
 #   Requires a private_key as well.
 #
 # [*ssl_private_key*]
 #   The name of the private key file within modules/private/files, if any.
 #   Requires a certificate as well.
 #
 # [*ensure*]
-#   Whether to set up the website or not.
+#   Whether to set up the website or not, e.g. "asbsent" or "present".
 #
 # [*deploy_user*]
 #   User that will be used to issue commands.
 #
 # [*deploy_user_authorized_keys*]
 #   Array of public keys that will have access to ssh commands
 #
 # [*hooks*]
 #   Hash of adblockplus::web::static::hook items to set up in this context.
 #
 # === Examples:
 #
 #   class {'adblockplus::web::static':
 #     domain => 'help.eyeo.com',
 #     hooks => {
-#       own-uname => {
+#       uname => {
 #         file => {
 #           content => 'uname -a',
-#         }
-#       }
+#         },
+#       },
+#       uptime => {
+#         file => {
+#           target => '/usr/bin/uptime',
+#           ensure => 'link',
+#         },
+#       },
 #     },
 #   }
 #
 class adblockplus::web::static (
   $domain,
   $ssl_certificate = undef,
   $ssl_private_key = undef,
   $ensure = 'present',
   $deploy_user = 'web-deploy',
-  $deploy_user_authorized_keys = undef,

[mathias, 2018/04/05 01:12:30]

This should default to an empty list.

[f.lopez, 2018/04/10 02:45:39]

Acknowledged.

+  $deploy_user_authorized_keys = [],
   $hooks = {},
 ) {
 
   include adblockplus::web
   include nginx
-  include geoip
   include ssh
 
   File {
     mode => '0755',
     owner => $deploy_user,
     group => $deploy_user,
   }
 
   ensure_resource('file', "/var/www/$domain", {
     ensure => ensure_directory_state($ensure),
-    owner => www-data,

[mathias, 2018/04/05 01:12:29]

Please quote non-keyword strings, always.

[f.lopez, 2018/04/10 02:45:38]

Acknowledged.

-    group => www-data,
+    owner => 'www-data',
+    group => 'www-data',
   })
 
   ensure_resource('nginx::hostconfig', $title, {
     content => template('adblockplus/web/static.conf.erb'),
     certificate => $ssl_certificate,
     domain => $domain,
-    is_default => 'true',

[mathias, 2018/04/05 01:12:29]

This keywords should not need quotes.

[f.lopez, 2018/04/10 02:45:38]

Acknowledged.

+    is_default => true,
     private_key => $ssl_private_key,
-    log => "web.access.log",

[mathias, 2018/04/05 01:12:28]

This should be single quotes. (I would prefer sticking to double ones
everywhere, but we don't want to derive from the communities standard coding
style without reason.)

[f.lopez, 2018/04/10 02:45:39]

Acknowledged.

+    log => 'web.access.log',
   })
 
   $content = [
     "Match User ${deploy_user}",
     'AllowTcpForwarding no',
     'X11Forwarding no',
     'AllowAgentForwarding no',
     'GatewayPorts no',
-    'ForceCommand /usr/local/bin/hooks_wrapper',
+    'ForceCommand /usr/local/bin/hooks_wrapper $SSH_ORIGINAL_COMMAND',
   ]
 
-  create_resources('concat::fragment', {

[mathias, 2018/04/05 01:12:28]

There's no need to use the create_resources() function here:
1. the user of the class cannot customize anything, and
2. there's no reason to assume that they would ever want to pre-define the
resource on the outside with the exact same but some more parameters either.

[f.lopez, 2018/04/10 02:45:38]

Acknowledged.

-  helpcenter => {
-    content => join($content, "\n"),

[mathias, 2018/04/05 01:12:28]

If you'd use "\n\t" or similar it would create a more readable result.

[f.lopez, 2018/04/10 02:45:38]

Acknowledged.

+  ensure_resource('concat::fragment', 'helpcenter', {
+    content => join($content, "\n\t"),
     ensure => 'present',
     target => 'sshd_config',
     order => '20',
-  }})
+  })
 
   ensure_resource('adblockplus::user', $deploy_user, {
     authorized_keys => $deploy_user_authorized_keys,
     ensure => $ensure,
-    password_hash => '*',

[mathias, 2018/04/05 01:12:28]

Is this parameter necessary? The underlying named type uses `undef` as default.

[f.lopez, 2018/04/10 02:45:39]

Acknowledged.

     shell => '/bin/bash',
     groups => ['www-data'],
   })
 
-  ensure_resource('file', "/usr/local/bin/hooks_wrapper", {

[mathias, 2018/04/05 01:12:28]

Double quotes again :)

[f.lopez, 2018/04/10 02:45:38]

Acknowledged.

+  $wrapper_path = "/home/${deploy_user}/bin"
+  ensure_resource('file', 'commands_dir', {
+    path => $wrapper_path,
+    ensure => ensure_directory_state($ensure),
+  })
+
+  ensure_resource('file', '/usr/local/bin/hooks_wrapper', {
     ensure => ensure_file_state($ensure),
     content => template('adblockplus/web/hooks_wrapper.sh.erb'),
-  })
-
-  ensure_resource('file', "/home/$deploy_user/bin", {

[mathias, 2018/04/05 01:12:30]

And again :)

[f.lopez, 2018/04/10 02:45:38]

this one has $deploy_user parameter in the middle, I can't remove the double
quotes but I can make more explicit the parameter

-    ensure => ensure_directory_state($ensure),
   })
 
   # https://docs.puppet.com/puppet/latest/function.html#createresources
   create_resources('adblockplus::web::static::hook', $hooks)
 }