Using shell "if" to conditionalize snippet inclusion is error-prone?

[Jun'ichi Nomura]

Hi,

In beaker 0.18, "rhts_post" snippet has a following construct:

if [ -f /etc/sysconfig/readahead ] ; then
  {% snippet 'readahead_sysconfig' %}
  fi

This is error-prone because if we have site-local "readahead_sysconfig"
snippet, which is empty, generated shell script will be broken.
(Actually, I've been using empty "readahead_sysconfig" snippet)

So it might be better to move if/then/fi into the snippet like below.

Same might apply to "virt_console_post".

---
Jun'ichi Nomura, NEC Corporation

diff --git a/Server/bkr/server/snippets/readahead_sysconfig b/Server/bkr/server/snippets/readahead_sysconfig
index 830f20b..393c215 100644
--- a/Server/bkr/server/snippets/readahead_sysconfig
+++ b/Server/bkr/server/snippets/readahead_sysconfig
@@ -1,3 +1,4 @@
+if [ -f /etc/sysconfig/readahead ] ; then
     cat >>/etc/sysconfig/readahead <<EOF

# readahead conflicts with auditd, see bug 561486 for detailed explanation.
@@ -8,3 +9,4 @@
 READAHEAD_COLLECT="no"
 READAHEAD_COLLECT_ON_RPM="no"
 EOF
+fi
diff --git a/Server/bkr/server/snippets/rhts_post b/Server/bkr/server/snippets/rhts_post
index bce2347..1635406 100644
--- a/Server/bkr/server/snippets/rhts_post
+++ b/Server/bkr/server/snippets/rhts_post
@@ -15,9 +15,7 @@ fi

# Enable post-install boot notification
 {% snippet 'post_anamon' %}
-if [ -f /etc/sysconfig/readahead ] ; then
 {% snippet 'readahead_sysconfig' %}
-fi
 {% snippet 'linkdelay' %}

{# We normally want to make sure the system time is accurate, in case

[Dan Callaghan]

Excerpts from Junichi Nomura's message of 2014-10-30 14:51 +10:00:
> Hi,
>
> In beaker 0.18, "rhts_post" snippet has a following construct:
>
> if [ -f /etc/sysconfig/readahead ] ; then
>   {% snippet 'readahead_sysconfig' %}
>   fi
>
> This is error-prone because if we have site-local "readahead_sysconfig"
> snippet, which is empty, generated shell script will be broken.
> (Actually, I've been using empty "readahead_sysconfig" snippet)
>
> So it might be better to move if/then/fi into the snippet like below.
>
> Same might apply to "virt_console_post".

Thanks for reporting this.

Ironically, the reason I kept the condition outside the snippet in the 
first place was to avoid breaking existing snippets :-) The previous 
version had the template conditional outside the snippets, like this:

{% if distro is osmajor('RedHatEnterpriseLinux6', 'RedHatEnterpriseLinuxGrid6') %}
    {% snippet 'readahead_sysconfig' %}
    {% endif %}

So if I moved the conditional inside the snippet, then a custom snippet 
would suddenly become unconditional (unless the admin also updated their 
custom snippet to add the conditional).

However I clearly didn't account for the fact that an empty snippet 
would make it a syntax error.

Probably the best solution is to just add a dummy statement after the 
"then" statement to make it syntactically valid even if the snippet is 
empty. I guess this should work:

if [ -f /etc/sysconfig/readahead ] ; then
        :
        {% snippet 'readahead_sysconfig' %}
    fi

[Dan Callaghan]

Excerpts from Dan Callaghan's message of 2014-10-31 11:41 +10:00:
> Excerpts from Junichi Nomura's message of 2014-10-30 14:51 +10:00:
> > This is error-prone because if we have site-local 
> > "readahead_sysconfig"
> > snippet, which is empty, generated shell script will be broken.
> > (Actually, I've been using empty "readahead_sysconfig" snippet)

[...]
> Probably the best solution is to just add a dummy statement after the 
> "then" statement to make it syntactically valid even if the snippet is 
> empty. I guess this should work:
> if [ -f /etc/sysconfig/readahead ] ; then
>     :
>     {% snippet 'readahead_sysconfig' %}
> fi

... and the workaround, which you probably already figured out, is to 
just make your custom snippet have a dummy statement instead of being 
completely empty.

[Jun'ichi Nomura]

On 10/31/14 10:41, Dan Callaghan wrote:
> Ironically, the reason I kept the condition outside the snippet in the 
> first place was to avoid breaking existing snippets :-) The previous 
> version had the template conditional outside the snippets, like this:
> {% if distro is osmajor('RedHatEnterpriseLinux6', 'RedHatEnterpriseLinuxGrid6') %}
> {% snippet 'readahead_sysconfig' %}
> {% endif %}
>
> So if I moved the conditional inside the snippet, then a custom snippet 
> would suddenly become unconditional (unless the admin also updated their 
> custom snippet to add the conditional).

Ah...
> However I clearly didn't account for the fact that an empty snippet 
> would make it a syntax error.
>
> Probably the best solution is to just add a dummy statement after the 
> "then" statement to make it syntactically valid even if the snippet is 
> empty. I guess this should work:
> if [ -f /etc/sysconfig/readahead ] ; then
>     :
>     {% snippet 'readahead_sysconfig' %}
> fi

Yeah, it seems the right solution.
Actually I changed my local "readahead_sysconfig" to just contain ":"
to work around the current situation.

[Dan Callaghan]

Excerpts from Dan Callaghan's message of 2014-10-31 11:41 +10:00:
> Probably the best solution is to just add a dummy statement after the 
> "then" statement to make it syntactically valid even if the snippet is 
> empty. I guess this should work:
> if [ -f /etc/sysconfig/readahead ] ; then
>     :
>     {% snippet 'readahead_sysconfig' %}
> fi

Filed with patch: https://bugzilla.redhat.com/show_bug.cgi?id=1160091