Merge lp:~lynxman/squid-deb-proxy/debhooks into lp:squid-deb-proxy

[Marc Cluet]

We added a couple of debconf hooks in order to be able to configure the config file directly from orchestra

[Michael Vogt (mvo)]

Thanks for this branch! I like the idea of adding debconf questions to make it easy to enable certain features. I think thats a great idea. I have some comments about the current implementation though.

[..]
> +case "$1" in
> + configure)
> + # Enable or disable PPAs
> + db_get squid-deb-proxy/ppa-enable
> + if [ "${RET}z" != "z" ]

Is this check for empty result really needed? db_get with a boolean
template should always return either "true" or "false"

> + then
> + case "$RET" in
> + n*|N*)

Did you test that this actually works? $RET should be either string
"true" or "false". The buttons in the question whiptail dialog are a
bit misleading as they can be translated, but the db_get value is
always either true or false.

> + if [ -f "/etc/squid-deb-proxy/mirror-dstdomain.acl" ]
> + then
> + mv /etc/squid-deb-proxy/mirror-dstdomain.acl /etc/squid-deb-proxy/.mirror-dstdomain.acl
> + cat /etc/squid-deb-proxy/.mirror-dstdomain.acl | sed 's/ppa.launchpad.net/#ppa.launchpad.net/' > /etc/squid-deb-proxy/mirror-dstdomain.acl
> + rm -f /etc/squid-deb-proxy/.mirror-dstdomain.acl

Is there a advantage to use this over sed -i ? sed will also
create a temp file first so it should be efffectively the same only
shorter (unless I overlook something here?).

There is one more issue with the approach taken. When running
$ dpkg -s squid-deb-proxy
[..]
Conffiles:
 /etc/squid-deb-proxy/mirror-dstdomain.acl c430d3bc4818751ee177515a41456708
[..]

This shows that the two files manipulated are "conffiles". The debian
policy (in appendix E.1) discourages the manipulation of conffiles
from a maintainer script. I think adding a new line to squid.conf
like
acl to_ubuntu_mirrors_user dstdomain "/etc/squid-deb-proxy/mirror-dstdomain-user.acl"
that is then either created empty or with the content like 0.0.0.0
should do the trick (for mirror-dstdomain-user.acl the same, either a
empty file or a file containing ppa). If that is carefully done all
users benefit as they can add their own content there without the
risks of having conffile prompts in the future. Alternatively it could
use include in the acl files. But I'm not sure that this will actually
work in a acl file.

[..]
> +Template: squid-deb-proxy/acl-enable
> +Type: boolean
> +Description: Enable network ACL
> + squid-deb-proxy restricts access to the cache by address range
> + selecting Y in this option will enable a basic private network range
> + selecting N in this option will allow all IPs to access the cache

Maybe something like "all IPs *in the world* to access the cache" or
something like this to ensure the user understands that this opens his
cache for all others. Not that big of a risk because all they can get
is packages but it may costs bandwith etc and the user/admin should
understand the difference.

review: Needs Fixing

[Marc Cluet (lynxman)]

Hi! *waves*

On Jun 7, 2011, at 16:03 , Michael Vogt wrote:

> Review: Needs Fixing
> Thanks for this branch! I like the idea of adding debconf questions to make it easy to enable certain features. I think thats a great idea. I have some comments about the current implementation though.
>
> [..]
>> +case "$1" in
>> + configure)
>> + # Enable or disable PPAs
>> + db_get squid-deb-proxy/ppa-enable
>> + if [ "${RET}z" != "z" ]
>
> Is this check for empty result really needed? db_get with a boolean
> template should always return either "true" or "false"

We've observed that not establishing a "default" will make the variable empty in some cases, that's why we implemented the check for empty result as "good practice" to avoid nasty surprises.

>
>> + then
>> + case "$RET" in
>> + n*|N*)
>
> Did you test that this actually works? $RET should be either string
> "true" or "false". The buttons in the question whiptail dialog are a
> bit misleading as they can be translated, but the db_get value is
> always either true or false.

It works, this example was actually straight from the package maintainer manual.

>
>> + if [ -f "/etc/squid-deb-proxy/mirror-dstdomain.acl" ]
>> + then
>> + mv /etc/squid-deb-proxy/mirror-dstdomain.acl /etc/squid-deb-proxy/.mirror-dstdomain.acl
>> + cat /etc/squid-deb-proxy/.mirror-dstdomain.acl | sed 's/ppa.launchpad.net/#ppa.launchpad.net/' > /etc/squid-deb-proxy/mirror-dstdomain.acl
>> + rm -f /etc/squid-deb-proxy/.mirror-dstdomain.acl
>
> Is there a advantage to use this over sed -i ? sed will also
> create a temp file first so it should be efffectively the same only
> shorter (unless I overlook something here?).

No there's no advantage over just using sed, it's just my disability to use sed without piping from cat, fixing this now

>
> There is one more issue with the approach taken. When running
> $ dpkg -s squid-deb-proxy
> [..]
> Conffiles:
> /etc/squid-deb-proxy/mirror-dstdomain.acl c430d3bc4818751ee177515a41456708
> [..]
>
> This shows that the two files manipulated are "conffiles". The debian
> policy (in appendix E.1) discourages the manipulation of conffiles
> from a maintainer script. I think adding a new line to squid.conf
> like
> acl to_ubuntu_mirrors_user dstdomain "/etc/squid-deb-proxy/mirror-dstdomain-user.acl"
> that is then either created empty or with the content like 0.0.0.0
> should do the trick (for mirror-dstdomain-user.acl the same, either a
> empty file or a file containing ppa). If that is carefully done all
> users benefit as they can add their own content there without the
> risks of having conffile prompts in the future. Alternatively it could
> use include in the acl files. But I'm not sure that this will actually
> work in a acl file.

I've tried that and the deny unfortunately takes precedence, could you give me an example that you'd think works properly?

>
>
> [..]
>> +Template: squid-deb-proxy/acl-enable
>> +Type: boolean
>> +Description: Enable network ACL
>> + squid-deb-proxy restricts access to the c...

=== added file 'debian/squid-deb-proxy.config'
--- debian/squid-deb-proxy.config	1970-01-01 00:00:00 +0000
+++ debian/squid-deb-proxy.config	2011-06-03 15:03:29 +0000
@@ -0,0 +1,7 @@
+#!/bin/sh -e
+# A simplified version of debconf's own config script.
+. /usr/share/debconf/confmodule
+# Ask for the rsyslog host
+db_input low squid-deb-proxy/ppa-enable || true
+db_input low squid-deb-proxy/acl-enable || true
+db_go

=== added file 'debian/squid-deb-proxy.postinst'
--- debian/squid-deb-proxy.postinst	1970-01-01 00:00:00 +0000
+++ debian/squid-deb-proxy.postinst	2011-06-03 15:03:29 +0000
@@ -0,0 +1,89 @@
+#!/bin/sh
+# postinst script for squid-deb-proxy
+#
+# see: dh_installdeb(1)
+
+set -e
+
+# summary of how this script can be called:
+#        * <postinst> `configure' <most-recently-configured-version>
+#        * <old-postinst> `abort-upgrade' <new version>
+#        * <conflictor's-postinst> `abort-remove' `in-favour' <package>
+#          <new-version>
+#        * <postinst> `abort-remove'
+#        * <deconfigured's-postinst> `abort-deconfigure' `in-favour'
+#          <failed-install-package> <version> `removing'
+#          <conflicting-package> <version>
+# for details, see http://www.debian.org/doc/debian-policy/ or
+# the debian-policy package
+
+# A simplified version of debconf's own config script.
+. /usr/share/debconf/confmodule
+
+case "$1" in
+    configure)
+
+        db_get squid-deb-proxy/ppa-enable
+        if [ "${RET}z" != "z" ]
+        then
+            case "$RET" in
+                n*|N*)
+                    if [ -f "/etc/squid-deb-proxy/mirror-dstdomain.acl" ]
+                        then
+                        mv -f /etc/squid-deb-proxy/mirror-dstdomain.acl /etc/squid-deb-proxy/.mirror-dstdomain.acl
+                        sed 's/ppa.launchpad.net/#ppa.launchpad.net/' /etc/squid-deb-proxy/.mirror-dstdomain.acl > /etc/squid-deb-proxy/mirror-dstdomain.acl
+                        rm -f /etc/squid-deb-proxy/.mirror-dstdomain.acl
+                    fi
+                    ;;
+                y*|Y*)
+                    if [ -f "/etc/squid-deb-proxy/mirror-dstdomain.acl" ]
+                        then
+                        mv -f /etc/squid-deb-proxy/mirror-dstdomain.acl /etc/squid-deb-proxy/.mirror-dstdomain.acl
+                        sed 's/#ppa.launchpad.net/ppa.launchpad.net/' /etc/squid-deb-proxy/.mirror-dstdomain.acl > /etc/squid-deb-proxy/mirror-dstdomain.acl
+                        rm -f /etc/squid-deb-proxy/.mirror-dstdomain.acl
+                    fi
+                    ;;
+            esac
+        fi
+
+        db_get squid-deb-proxy/acl-enable
+        if [ "${RET}z" != "z" ]
+        then
+            case "$RET" in
+                n*|N*)
+                    if [ -f "/etc/squid-deb-proxy/squid-deb-proxy.conf" ]
+                    then
+                        mv -f /etc/squid-deb-proxy/squid-deb-proxy.conf /etc/squid-deb-proxy/.squid-deb-proxy.conf
+                        sed 's/http_access deny all/http_access allow all/' /etc/squid-deb-proxy/.squid-deb-proxy.conf > /etc/squid-deb-proxy/squid-deb-proxy.conf
+                        rm -f /etc/squid-deb-proxy/.squid-deb-proxy.conf
+                    fi
+                    ;;
+                y*|Y*)
+                    if [ -f "/etc/squid-deb-proxy/squid-deb-proxy.conf" ]
+                    then
+                        mv -f /etc/squid-deb-proxy/squid-deb-proxy.conf /etc/squid-deb-proxy/.squid-deb-proxy.conf
+                        sed 's/http_access allow all/http_access deny all/' /etc/squid-deb-proxy/.squid-deb-proxy.conf > /etc/squid-deb-proxy/squid-deb-proxy.conf
+                        rm -f /etc/squid-deb-proxy/.squid-deb-proxy.conf
+                    fi
+                    ;;
+            esac
+        fi
+
+        invoke-rc.d squid-deb-proxy restart
+    ;;
+
+    abort-upgrade|abort-remove|abort-deconfigure|triggered)
+    ;;
+
+    *)
+        echo "postinst called with unknown argument \`$1'" >&2
+        exit 1
+    ;;
+esac
+
+# dh_installdeb will replace this with shell code automatically
+# generated by other debhelper scripts.
+
+#DEBHELPER#
+
+exit 0

=== added file 'debian/squid-deb-proxy.templates'
--- debian/squid-deb-proxy.templates	1970-01-01 00:00:00 +0000
+++ debian/squid-deb-proxy.templates	2011-06-03 15:03:29 +0000
@@ -0,0 +1,12 @@
+Template: squid-deb-proxy/ppa-enable
+Type: boolean
+Description: Enable PPA caching
+   squid-deb-proxy by default will not cache ppa repositories
+   selecting Y in this option will activate ppa repo caching
+
+Template: squid-deb-proxy/acl-enable
+Type: boolean
+Description: Enable network ACL
+   squid-deb-proxy restricts access to the cache by address range
+   selecting Y in this option will enable a basic private network range
+   selecting N in this option will allow all IPs to access the cache