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

Delta Between Two Patch Sets: modules/adblockplus/templates/web/hooks_wrapper.sh.erb

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

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

Right Patch Set: Make documentation right Created April 17, 2018, 8:03 p.m.

Left:
Right:

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

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 #!/bin/sh	1 #!/bin/sh

	2 # Puppet: <%= @title %>

	3 # This script is a wrapper of accesible hooks, defined via hiera and accessed

	4 # via ssh with the ForceCommand option.

2	5

3 # This script is a wrapper of accesible commands via ssh ForceCommand option	6 usage()
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	7 {
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.
	8 echo "Avaiable commands are:"

	9 echo "help"

	10 <% @hooks.keys.each do \|key\| -%>

	11 echo "<%= key %>"

	12 <% end -%>

	13 }

	14

5 command="$1"; shift	15 command="$1"; shift

6 case "$command" in	16 case "$command" in

7 <%- @hooks.each do \|command, values\| -%>	17 "help")
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 %>)	18 usage

9 /home/<%= @deploy_user %>/bin/<%= command %> "$@"	19 ;;
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 ;;	20 <% @hooks.each do \|command, values\| -%>

11 <%- end -%>	21 "<%= command %>")

12 *)	22 "<%= File.join(@wrapper_path, command) %>" "$@"

13 echo "Sorry. Only these commands are available to you:"	23 ;;
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\| -%>	24 <% end -%>
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 %>	25 *)

16 <%- end -%>	26 echo "Unrecognized command"

17 exit 1	27 usage

18 ;;	28 exit 1

	29 ;;

19 esac	30 esac

	31

LEFT	RIGHT