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

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.

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...