modules/adblockplus/templates/web/hooks_wrapper.sh.erb - Issue 29733731: #7320 - Introduce helpcenter role

Side by Side Diff: modules/adblockplus/templates/web/hooks_wrapper.sh.erb

Issue 29733731: #7320 - Introduce helpcenter role (Closed)

Patch Set: For comments 2 to 4 Created April 2, 2018, 10:36 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

« modules/adblockplus/manifests/web/static/hook.pp ('K') | « modules/adblockplus/manifests/web/static/hook.pp ('k') | modules/adblockplus/templates/web/static.conf.erb » ('j') | modules/adblockplus/templates/web/static.conf.erb » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 #!/bin/sh

	2

	3 # This script is a wrapper of accesible commands via ssh ForceCommand option
	mathias 2018/04/05 01:12:31 This should mention that it is to ensure only the 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. f.lopez 2018/04/10 02:45:40 Acknowledged. Show quoted text 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.
	4 set -- $SSH_ORIGINAL_COMMAND
	mathias 2018/04/05 01:12:30 Instead of replacing "$@" here you should pass on 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? f.lopez 2018/04/10 02:45:40 Acknowledged. Show quoted text 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.
	5 command="$1"; shift

	6 case "$command" in

	7 <%- @hooks.each do \|command, values\| -%>
	mathias 2018/04/05 01:12:31 I prefer when the template brackets (`<%`) are use 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. f.lopez 2018/04/10 02:45:40 Acknowledged. Show quoted text 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.
	8 <%= command %>)

	9 /home/<%= @deploy_user %>/bin/<%= command %> "$@"
	mathias 2018/04/05 01:12:31 This should not repeat the convention from hook.pp 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. f.lopez 2018/04/10 02:45:39 Acknowledged. Show quoted text 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.
	10 ;;

	11 <%- end -%>

	12 *)

	13 echo "Sorry. Only these commands are available to you:"
	mathias 2018/04/05 01:12:31 Please don't use personal articulation, stuff like 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. f.lopez 2018/04/10 02:45:40 Acknowledged. Show quoted text 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.
	14 <%- @hooks.keys.select { \|key\| key.to_s}.each do \|value\| -%>
	mathias 2018/04/05 01:12:30 Aren't the key's strings anyway? And even if not, Aren't the key's strings anyway? And even if not, aren't they printable? f.lopez 2018/04/10 02:45:40 Acknowledged. Show quoted text 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.
	15 echo <%= value %>

	16 <%- end -%>

	17 exit 1

	18 ;;

	19 esac

OLD	NEW