Issue 29733731: #7320 - Introduce helpcenter role (Closed)
Description#7320 - Introduce helpcenter role
Patch Set 1 #
Total comments: 38
Patch Set 2 : For comments 2 to 4 #
Total comments: 44
Patch Set 3 : For comments 5 and 6 #
Total comments: 8
Patch Set 4 : For comments 8 and 9 #
Total comments: 4
Patch Set 5 : For comments 11 and 12 #Patch Set 6 : A bit more documentation #Patch Set 7 : Make documentation right #
MessagesTotal messages: 16
https://codereview.adblockplus.org/29733731/diff/29733732/hiera/roles/web/sta... File hiera/roles/web/static/helpcenter.yaml (right): https://codereview.adblockplus.org/29733731/diff/29733732/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:6: - "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACcgC+yCp66GDc1iWCXRv/mZslxfG83uUHdOtPiOA2qtZwj6KheWbfTgB2KcV4MiXrgIT/ls8w43zZf4mhVwNXnW295v6i24qAC6qjL08M134MgcCuS+Gz+Ma30YZUwT9ZQWJRp4y9UnZbXQ+Nkl0bS2XQrVbGWP0VnIbXNtoUYb/XOm+twnccgVPsqMg9AAcVKPINrcW+2Lr77k44K8nKqbJZzb5YrQZhpj1GH3Wx/TrluzJKGVJpBz7st/gt8ulLR5ETeQAvNwAV0/h4/3+9P93euYrOzpjzDTusMOuxQ8npFyTOd4Bx56DugaLkQsbR4bu3T7kYynrVqD6MK/zxLs6Ipn3+juwybb5J5HNZKMAten+7MOA8oMF7VfIwb9Nppt8JOqSO4YoAfwCCSCvYj92JMJP8Ko0LbnAyVRmc70218jaQk+Gr7Y/TYTseEGAVa/z3emvHc7tH2osyh+IX/WnIR+tApaWd1kTS2Xifo5i91pQbwvBZRccL8YUTP97pEkNDNe0LXesyEgCtepe4qFIg2x2oHgx8Y8PmC+XwY0cJ/z6qZ6nUL4l2ccVcA5Z/o27Vaj4zmVgGSOaukkdCE7AFlTa4EIrATjlInhhYLgOBGII7zJE6HOcqufOPJnlDY5VhYeWCShSdF1d0kMl11QYOIz2CYr07SlVD/Upn7z927JKWHCfAeYb/uhJwb2eF92VluIdzLkTMO3Icv1U4Vxvif2YR2vfur3PhvDXrBuxCW8h8wLXrhOJ4Njx4MiBj3Ym4Dv+KZCaxd+Aq14HMB+EdVsFl4apm4zwakzq9dudpv1DAFgr3c8bVnNclDRxXGQEL user@Client067" Whom does that key belong to and why are they so important that we need to include them in our code base? https://codereview.adblockplus.org/29733731/diff/29733732/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:14: helpcenter: This should be part of the puppet code, not something we have to cargo-cult throughout the various setups. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/manifests/web/static.pp (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:8: # === Parameters: Incomplete. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:34: # content => 'uname -a', That is a quite strange "deploy" example. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:41: $domain = undef, A required parameter shall not default to anything, not even undef. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:66: is_default => $is_default, is_undef https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:68: log => "access_log_$domain", I don't know who started this practice but I don't like it: Either call it access_log_web_static or something or call it "$domain.access" or "access.$domain" or add ".log" or whatever - but please don't mix different notations, e.g. underscores and file-ext-agnostic-dots ("access_log_help.eyeo.com" is just a shitty file name). Also, as long as this is a class and not a named type there is no point in having a variable log file name. Just makes configuration more prone to error. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/manifests/web/static/hook.pp (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:17: # 'own-uname' => { This seems like a fragment that doesn't belong here. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:32: ensure_resource('file', "/usr/local/bin/commands", { You shall not create a global command called "commands". Use a more distinct (i.e. specific) name. Also, as long as adblockplus::web::static is a class, this resource is better placed there. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:37: ensure_resource('file', "/home/$adblockplus::web::static::deploy_user/bin", { As long as adblockplus::web::static is a class this resource is better placed there. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:41: ensure_resource('file', Now which convention was the base for the kind of indentation that follows below this line? https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:42: "script#${name}", Using "script#" as a local context prefix for file/script/whatever resources is pretty much pointless. Unless the surrounding class or define is called "script", of course. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:46: }, $file)) The "path" should be merged after $file (maybe "ensure" as well). https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/templates/web/commands.sh.erb (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/commands.sh.erb:1: #!/bin/sh This script is either horribly implemented or pointless and horribly implemented. Thus, before we can provide you with proper feedback, please document the code. Thank you! https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/templates/web/static.conf.erb (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:1: # Puppet: <%= @title %> Please don't simply copy from our legacy. There's many improvements possible here, and no need to inherit everything. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:11: <%= @custom_config %> We know the two spaces in front of the custom configuration to be irritating, to say the least, and pointless anyway. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:27: set $preferredRegion ""; Please avoid camel case where possible, and always try to stay consistent with the surrounding code. Here: use underscores. Also the "preferred" prefix is pointless because the variable is not used to identify the "preferred" language. Use "$language" and "$region" directly or, if you want to use a prefix to avoid collisions, something that is either generic ("$custom_language") or precise ("$requested_language" or "$client_language") https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:28: if ($http_accept_language ~ ^(\w\w)\b) An empty line in front of this block would be appreciated. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:61: location ~ ^/([a-z][a-z]\_[A-Z][A-Z])(/.+) Why use [a-z] here but \w above? Why use [A-Z] here but \w above?
https://codereview.adblockplus.org/29733731/diff/29733732/hiera/roles/web/sta... File hiera/roles/web/static/helpcenter.yaml (right): https://codereview.adblockplus.org/29733731/diff/29733732/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:6: - "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACcgC+yCp66GDc1iWCXRv/mZslxfG83uUHdOtPiOA2qtZwj6KheWbfTgB2KcV4MiXrgIT/ls8w43zZf4mhVwNXnW295v6i24qAC6qjL08M134MgcCuS+Gz+Ma30YZUwT9ZQWJRp4y9UnZbXQ+Nkl0bS2XQrVbGWP0VnIbXNtoUYb/XOm+twnccgVPsqMg9AAcVKPINrcW+2Lr77k44K8nKqbJZzb5YrQZhpj1GH3Wx/TrluzJKGVJpBz7st/gt8ulLR5ETeQAvNwAV0/h4/3+9P93euYrOzpjzDTusMOuxQ8npFyTOd4Bx56DugaLkQsbR4bu3T7kYynrVqD6MK/zxLs6Ipn3+juwybb5J5HNZKMAten+7MOA8oMF7VfIwb9Nppt8JOqSO4YoAfwCCSCvYj92JMJP8Ko0LbnAyVRmc70218jaQk+Gr7Y/TYTseEGAVa/z3emvHc7tH2osyh+IX/WnIR+tApaWd1kTS2Xifo5i91pQbwvBZRccL8YUTP97pEkNDNe0LXesyEgCtepe4qFIg2x2oHgx8Y8PmC+XwY0cJ/z6qZ6nUL4l2ccVcA5Z/o27Vaj4zmVgGSOaukkdCE7AFlTa4EIrATjlInhhYLgOBGII7zJE6HOcqufOPJnlDY5VhYeWCShSdF1d0kMl11QYOIz2CYr07SlVD/Upn7z927JKWHCfAeYb/uhJwb2eF92VluIdzLkTMO3Icv1U4Vxvif2YR2vfur3PhvDXrBuxCW8h8wLXrhOJ4Njx4MiBj3Ym4Dv+KZCaxd+Aq14HMB+EdVsFl4apm4zwakzq9dudpv1DAFgr3c8bVnNclDRxXGQEL user@Client067" On 2018/03/27 14:46:50, mathias wrote: > Whom does that key belong to and why are they so important that we need to > include them in our code base? Forgot to remove https://codereview.adblockplus.org/29733731/diff/29733732/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:14: helpcenter: On 2018/03/27 14:46:50, mathias wrote: > This should be part of the puppet code, not something we have to cargo-cult > throughout the various setups. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/manifests/web/static.pp (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:8: # === Parameters: On 2018/03/27 14:46:51, mathias wrote: > Incomplete. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:34: # content => 'uname -a', On 2018/03/27 14:46:51, mathias wrote: > That is a quite strange "deploy" example. heh :), I'll change the name of the example :D https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:41: $domain = undef, On 2018/03/27 14:46:50, mathias wrote: > A required parameter shall not default to anything, not even undef. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:66: is_default => $is_default, On 2018/03/27 14:46:50, mathias wrote: > is_undef Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:68: log => "access_log_$domain", On 2018/03/27 14:46:51, mathias wrote: > I don't know who started this practice but I don't like it: Either call it > access_log_web_static or something or call it "$domain.access" or > "access.$domain" or add ".log" or whatever - but please don't mix different > notations, e.g. underscores and file-ext-agnostic-dots > ("access_log_help.eyeo.com" is just a shitty file name). Also, as long as this > is a class and not a named type there is no point in having a variable log file > name. Just makes configuration more prone to error. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/manifests/web/static/hook.pp (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:17: # 'own-uname' => { On 2018/03/27 14:46:51, mathias wrote: > This seems like a fragment that doesn't belong here. Oh, I didn't delete the other example, I'll fix it https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:32: ensure_resource('file', "/usr/local/bin/commands", { On 2018/03/27 14:46:51, mathias wrote: > You shall not create a global command called "commands". Use a more distinct > (i.e. specific) name. Also, as long as adblockplus::web::static is a class, this > resource is better placed there. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:37: ensure_resource('file', "/home/$adblockplus::web::static::deploy_user/bin", { On 2018/03/27 14:46:51, mathias wrote: > As long as adblockplus::web::static is a class this resource is better placed > there. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:41: ensure_resource('file', On 2018/03/27 14:46:51, mathias wrote: > Now which convention was the base for the kind of indentation that follows below > this line? The line would be 80+ if I didn't split it up, so I decided to put it that way. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:42: "script#${name}", On 2018/03/27 14:46:51, mathias wrote: > Using "script#" as a local context prefix for file/script/whatever resources is > pretty much pointless. Unless the surrounding class or define is called > "script", of course. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:46: }, $file)) On 2018/03/27 14:46:51, mathias wrote: > The "path" should be merged after $file (maybe "ensure" as well). Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/templates/web/commands.sh.erb (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/commands.sh.erb:1: #!/bin/sh On 2018/03/27 14:46:52, mathias wrote: > This script is either horribly implemented or pointless and horribly > implemented. Thus, before we can provide you with proper feedback, please > document the code. Thank you! Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/templates/web/static.conf.erb (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:11: <%= @custom_config %> On 2018/03/27 14:46:52, mathias wrote: > We know the two spaces in front of the custom configuration to be irritating, to > say the least, and pointless anyway. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:27: set $preferredRegion ""; On 2018/03/27 14:46:52, mathias wrote: > Please avoid camel case where possible, and always try to stay consistent with > the surrounding code. Here: use underscores. Also the "preferred" prefix is > pointless because the variable is not used to identify the "preferred" language. > Use "$language" and "$region" directly or, if you want to use a prefix to avoid > collisions, something that is either generic ("$custom_language") or precise > ("$requested_language" or "$client_language") Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:28: if ($http_accept_language ~ ^(\w\w)\b) On 2018/03/27 14:46:52, mathias wrote: > An empty line in front of this block would be appreciated. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:61: location ~ ^/([a-z][a-z]\_[A-Z][A-Z])(/.+) On 2018/03/27 14:46:52, mathias wrote: > Why use [a-z] here but \w above? > Why use [A-Z] here but \w above? Not sure, gonna try it with \w, I reused what we already had, so I'll try and change if possible or tell you why we didn't use \w
https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... File modules/adblockplus/templates/web/static.conf.erb (right): https://codereview.adblockplus.org/29733731/diff/29733732/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:61: location ~ ^/([a-z][a-z]\_[A-Z][A-Z])(/.+) On 2018/03/27 14:46:52, mathias wrote: > Why use [a-z] here but \w above? > Why use [A-Z] here but \w above? This is because how the translations pages are built and looked for: a translation can be: `es` for spanish or `es_MX` for mexican spanish so this code looks exactly for lower and upper letters, instead of just letters in order to avoid confusion.
https://codereview.adblockplus.org/29733731/diff/29740568/hiera/roles/web/sta... File hiera/roles/web/static/helpcenter.yaml (right): https://codereview.adblockplus.org/29733731/diff/29740568/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:5: hooks: One cannot overwrite the `hooks` unless they're given in their fully-qualified form at root level of the configuration, i.e. `adblockplus::web::static::hooks`. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/manifests/web/static.pp (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:6: # There's one empty line too many here. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:52: $deploy_user_authorized_keys = undef, This should default to an empty list. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:69: owner => www-data, Please quote non-keyword strings, always. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:77: is_default => 'true', This keywords should not need quotes. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:79: log => "web.access.log", 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.) https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:91: create_resources('concat::fragment', { 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. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:93: content => join($content, "\n"), If you'd use "\n\t" or similar it would create a more readable result. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:102: password_hash => '*', Is this parameter necessary? The underlying named type uses `undef` as default. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:107: ensure_resource('file', "/usr/local/bin/hooks_wrapper", { Double quotes again :) https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:112: ensure_resource('file', "/home/$deploy_user/bin", { And again :) https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/manifests/web/static/hook.pp (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:8: # Overwrite the default configuration for the hook. This is not a default but totally necessary, in the current version that is. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:29: ensure_resource('file', "hook#${name}", merge({ When composing titles, which are required to be unique per resource type, you should use $title for that exact reason. Not $name, which only defaults to $title but is not restricted in the same fashion. Also the prefix "hook" is not really distinct enough: It is totally possible to have a "hook#test" in this setup and some auxiliary system, i.e. monitoring or something, on the same node. A "web-deploy-hook#test" and "monitor-alert-hook#test" would be way better, right? https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:31: owner =>$adblockplus::web::static::deploy_user, There's a space missing here. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:35: path => "/home/$adblockplus::web::static::deploy_user/bin/${name}", Both variables should be denoted with the `${}` syntax. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/templates/web/hooks_wrapper.sh.erb (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:3: # This script is a wrapper of accesible commands via ssh ForceCommand option This should mention that it is to ensure only the defined hooks are actually accessible. It should also mention where it comes from (`# ... generated in Puppet: <%= @title %>`). And the description should be followed by an empty line, not preceded. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:4: set -- $SSH_ORIGINAL_COMMAND Instead of replacing "$@" here you should pass on $SSH_ORIGINAL_COMMAND to the script when it is invoked by / configured with ForceCommand in static.pp -- or instead of replacing the scripts stack you should make sure it has the correct one when it is invoked -- or why the heck does the script need to know it is executed from SSH? https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:7: <%- @hooks.each do |command, values| -%> I prefer when the template brackets (`<%`) are used in the beginning of the line, not the whitespace getting deleted using the minus modifier. This allows for indentation on the programmatic level, i.e.: <% @a.each do |b| -%> <% b.each.do |fooobar| -%> .... <% end -%> <% end -%> It also makes identifying template logic lines more easy. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:9: /home/<%= @deploy_user %>/bin/<%= command %> "$@" This should not repeat the convention from hook.pp (creating a 2nd point of failure). Either use a shared variable in both places or `getparam(File["...#$command"], 'path')`. And quotes around the stuff are probably useful as well, a path can contain white-space after all. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:13: echo "Sorry. Only these commands are available to you:" Please don't use personal articulation, stuff like "sorry" and "you". Stick to the facts, i.e. carp about an "unrecognized command" and offer to try a "help" command. Speaking of which, that is also somehow required because otherwise you force the user to trigger an error just to get an overview. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:14: <%- @hooks.keys.select { |key| key.to_s}.each do |value| -%> Aren't the key's strings anyway? And even if not, aren't they printable? https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/templates/web/static.conf.erb (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:26: set $lang en; Abbreviation without any reason? https://www.youtube.com/watch?v=oKI-tD0L18A
https://codereview.adblockplus.org/29733731/diff/29740568/hiera/roles/web/sta... File hiera/roles/web/static/helpcenter.yaml (right): https://codereview.adblockplus.org/29733731/diff/29740568/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:5: hooks: On 2018/04/05 01:12:28, mathias wrote: > One cannot overwrite the `hooks` unless they're given in their fully-qualified > form at root level of the configuration, i.e. `adblockplus::web::static::hooks`. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/manifests/web/static.pp (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:6: # On 2018/04/05 01:12:30, mathias wrote: > There's one empty line too many here. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:52: $deploy_user_authorized_keys = undef, On 2018/04/05 01:12:30, mathias wrote: > This should default to an empty list. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:69: owner => www-data, On 2018/04/05 01:12:29, mathias wrote: > Please quote non-keyword strings, always. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:77: is_default => 'true', On 2018/04/05 01:12:29, mathias wrote: > This keywords should not need quotes. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:79: log => "web.access.log", On 2018/04/05 01:12:28, mathias wrote: > 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.) Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:91: create_resources('concat::fragment', { On 2018/04/05 01:12:28, mathias wrote: > 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. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:93: content => join($content, "\n"), On 2018/04/05 01:12:28, mathias wrote: > If you'd use "\n\t" or similar it would create a more readable result. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:102: password_hash => '*', On 2018/04/05 01:12:28, mathias wrote: > Is this parameter necessary? The underlying named type uses `undef` as default. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:107: ensure_resource('file', "/usr/local/bin/hooks_wrapper", { On 2018/04/05 01:12:28, mathias wrote: > Double quotes again :) Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:112: ensure_resource('file', "/home/$deploy_user/bin", { On 2018/04/05 01:12:30, mathias wrote: > And again :) this one has $deploy_user parameter in the middle, I can't remove the double quotes but I can make more explicit the parameter https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/manifests/web/static/hook.pp (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:8: # Overwrite the default configuration for the hook. On 2018/04/05 01:12:30, mathias wrote: > This is not a default but totally necessary, in the current version that is. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:29: ensure_resource('file', "hook#${name}", merge({ On 2018/04/05 01:12:30, mathias wrote: > When composing titles, which are required to be unique per resource type, you > should use $title for that exact reason. Not $name, which only defaults to > $title but is not restricted in the same fashion. > > Also the prefix "hook" is not really distinct enough: It is totally possible to > have a "hook#test" in this setup and some auxiliary system, i.e. monitoring or > something, on the same node. A "web-deploy-hook#test" and > "monitor-alert-hook#test" would be way better, right? Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:31: owner =>$adblockplus::web::static::deploy_user, On 2018/04/05 01:12:30, mathias wrote: > There's a space missing here. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:35: path => "/home/$adblockplus::web::static::deploy_user/bin/${name}", On 2018/04/05 01:12:30, mathias wrote: > Both variables should be denoted with the `${}` syntax. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/templates/web/hooks_wrapper.sh.erb (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:3: # This script is a wrapper of accesible commands via ssh ForceCommand option On 2018/04/05 01:12:31, mathias wrote: > This should mention that it is to ensure only the defined hooks are actually > accessible. It should also mention where it comes from (`# ... generated in > Puppet: <%= @title %>`). And the description should be followed by an empty > line, not preceded. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:4: set -- $SSH_ORIGINAL_COMMAND On 2018/04/05 01:12:30, mathias wrote: > Instead of replacing "$@" here you should pass on $SSH_ORIGINAL_COMMAND to the > script when it is invoked by / configured with ForceCommand in static.pp -- or > instead of replacing the scripts stack you should make sure it has the correct > one when it is invoked -- or > why the heck does the script need to know it is executed from SSH? Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:7: <%- @hooks.each do |command, values| -%> On 2018/04/05 01:12:31, mathias wrote: > I prefer when the template brackets (`<%`) are used in the beginning of the > line, not the whitespace getting deleted using the minus modifier. This allows > for indentation on the programmatic level, i.e.: > > <% @a.each do |b| -%> > <% b.each.do |fooobar| -%> > .... > <% end -%> > <% end -%> > > It also makes identifying template logic lines more easy. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:9: /home/<%= @deploy_user %>/bin/<%= command %> "$@" On 2018/04/05 01:12:31, mathias wrote: > This should not repeat the convention from hook.pp (creating a 2nd point of > failure). Either use a shared variable in both places or > `getparam(File["...#$command"], 'path')`. And quotes around the stuff are > probably useful as well, a path can contain white-space after all. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:13: echo "Sorry. Only these commands are available to you:" On 2018/04/05 01:12:31, mathias wrote: > Please don't use personal articulation, stuff like "sorry" and "you". Stick to > the facts, i.e. carp about an "unrecognized command" and offer to try a "help" > command. Speaking of which, that is also somehow required because otherwise you > force the user to trigger an error just to get an overview. Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:14: <%- @hooks.keys.select { |key| key.to_s}.each do |value| -%> On 2018/04/05 01:12:30, mathias wrote: > Aren't the key's strings anyway? And even if not, aren't they printable? Acknowledged. https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... File modules/adblockplus/templates/web/static.conf.erb (right): https://codereview.adblockplus.org/29733731/diff/29740568/modules/adblockplus... modules/adblockplus/templates/web/static.conf.erb:26: set $lang en; On 2018/04/05 01:12:31, mathias wrote: > Abbreviation without any reason? https://www.youtube.com/watch?v=oKI-tD0L18A Acknowledged.
https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... File modules/adblockplus/manifests/web/static.pp (right): https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:21: # Whether to set up the website or not. This should mention possible values, e.g. "absent" or "present". https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:57: include geoip Why? https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... File modules/adblockplus/templates/web/hooks_wrapper.sh.erb (right): https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:20: <% @hooks.each do |command, values| -%> Why the double space here? https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:22: <%= scope.function_join([[@wrapper_path, command], "/"]) %> "$@" Why not use Ruby's very own join method here? Also the path to the hook should be in quotes.
https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... File modules/adblockplus/manifests/web/static.pp (right): https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:21: # Whether to set up the website or not. On 2018/04/17 15:57:13, mathias wrote: > This should mention possible values, e.g. "absent" or "present". Acknowledged. https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/manifests/web/static.pp:57: include geoip On 2018/04/17 15:57:13, mathias wrote: > Why? you are right, there is no need for this just yet. https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... File modules/adblockplus/templates/web/hooks_wrapper.sh.erb (right): https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:20: <% @hooks.each do |command, values| -%> On 2018/04/17 15:57:13, mathias wrote: > Why the double space here? Acknowledged. https://codereview.adblockplus.org/29733731/diff/29747666/modules/adblockplus... modules/adblockplus/templates/web/hooks_wrapper.sh.erb:22: <%= scope.function_join([[@wrapper_path, command], "/"]) %> "$@" On 2018/04/17 15:57:14, mathias wrote: > Why not use Ruby's very own join method here? > Also the path to the hook should be in quotes. Acknowledged.
https://codereview.adblockplus.org/29733731/diff/29754571/hiera/roles/web/sta... File hiera/roles/web/static/helpcenter.yaml (right): https://codereview.adblockplus.org/29733731/diff/29754571/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:9: content: "uname -a" Couldn't that one use `target: /bin/uname` and `ensure: link`? https://codereview.adblockplus.org/29733731/diff/29754571/modules/adblockplus... File modules/adblockplus/manifests/web/static/hook.pp (right): https://codereview.adblockplus.org/29733731/diff/29754571/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:21: # content => 'uname -a', Maybe here as well?
https://codereview.adblockplus.org/29733731/diff/29754571/hiera/roles/web/sta... File hiera/roles/web/static/helpcenter.yaml (right): https://codereview.adblockplus.org/29733731/diff/29754571/hiera/roles/web/sta... hiera/roles/web/static/helpcenter.yaml:9: content: "uname -a" On 2018/04/17 19:28:04, mathias wrote: > Couldn't that one use `target: /bin/uname` and `ensure: link`? Acknowledged. https://codereview.adblockplus.org/29733731/diff/29754571/modules/adblockplus... File modules/adblockplus/manifests/web/static/hook.pp (right): https://codereview.adblockplus.org/29733731/diff/29754571/modules/adblockplus... modules/adblockplus/manifests/web/static/hook.pp:21: # content => 'uname -a', On 2018/04/17 19:28:04, mathias wrote: > Maybe here as well? Acknowledged.
lgtm. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
