Code review comment for lp://staging/~lynxman/squid-deb-proxy/debhooks
Revision history for this message
| Marc Cluet (lynxman) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
2eb648a
demo site
(Get the code!)
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-
>> + 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-
>> + then
>> + mv /etc/squid-
>> + cat /etc/squid-
>> + rm -f /etc/squid-
>
> 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-
> [..]
>
> 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_
> that is then either created empty or with the content like 0.0.0.0
> should do the trick (for mirror-
> 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-
>> +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.
Completely true, fixing
>
> --
> https:/
> You are the owner of lp:~lynxman/squid-deb-proxy/debhooks.
--
Marc Cluet <email address hidden>
System Integration Engineer
Canonical Ltd.
Office: +44 20 7630 2445
Mobile: +44 7593 233 734
GPG : 31AD 2628 3D54 BC6D 041B 774E 6A3A 01DC 3A15 C5A8