Merge lp://qastaging/~dpb/orange-box/use-nc-not-ping-1361714 into lp://qastaging/orange-box

Review: Needs Fixing

On Wed, Aug 27, 2014 at 6:10 PM, David Britton
<email address hidden> wrote:
> David Britton has proposed merging lp:~davidpbritton/orange-box/use-nc-not-ping-1361714 into lp:orange-box.
>
> Requested reviews:
> Orange Box (orange-box)
> Related bugs:
> Bug #1361714 in Orange Box: "orange-box postinst uses ping to test connectivity to maas.ubuntu.com"
> https://bugs.launchpad.net/orange-box/+bug/1361714
>
> For more details, see:
> https://code.launchpad.net/~davidpbritton/orange-box/use-nc-not-ping-1361714/+merge/232490
>
> Introduce new script ob-test-uplink and call this instead of using icmp ping.
>
> Please let me know if you would like any more hosts added.
> --
> https://code.launchpad.net/~davidpbritton/orange-box/use-nc-not-ping-1361714/+merge/232490
> You are subscribed to branch lp:orange-box.
>
> === modified file 'debian/postinst'
> --- debian/postinst 2014-08-25 16:40:02 +0000
> +++ debian/postinst 2014-08-27 23:09:22 +0000
> @@ -37,7 +37,8 @@
> fi
> # Setup nat
> # Ensure we have an external connection
> - run-one-until-success ping -c 3 maas.ubuntu.com
> + orange-box-test-uplink
> + run-one-until-success

This isn't quite right. You would need to put these both on the same
line, like this:

run-one-until-success orange-box-test-uplink

Also, check that you're using tabs here, and not 8 white spaces.

> orange-box-setup-nat
> # Redirect to MAAS web interface
> cat >/var/www/html/index.html <<EOF
> @@ -96,8 +97,7 @@
> # MAAS won't work very well until this is done, so we're going to block
> # until this completes
> # Network access may not be immediately available;
> - # wait until we can talk to maas
> - run-one-until-success ping -c 3 maas.ubuntu.com
> + orange-box-test-uplink

Use a tab. And wrap with run-one-until-success.

> # Support MAAS 1.5, and 1.6
> maas_ver=$(dpkg -l maas | tail -n1 | awk '{print $3}')
> if dpkg --compare-versions $maas_ver lt 1.6; then
>
> === added file 'usr/bin/orange-box-test-uplink'
> --- usr/bin/orange-box-test-uplink 1970-01-01 00:00:00 +0000
> +++ usr/bin/orange-box-test-uplink 2014-08-27 23:09:22 +0000
> @@ -0,0 +1,46 @@
> +#!/bin/sh
> +#
> +# orange-box-amt-recover - recover a dead AMT by dropping all connections

Thanks for copying the template. Make sure you update this line
appropriately ;-)

> +# Copyright (C) 2014 Canonical Ltd.
> +#
> +# Authors: David Britton <email address hidden>
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, version 3 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www...

Thanks Dustin --

I believe I've addressed all the feedback except one thing,

orange-box-test-uplink already does a timeout and run-one-until-success, you would like another wrapper around that? You think there should be a timeout associated too? One issue I noticed is that it was waiting forever which seemed odd for a postinst.

Thoughts?

And ya, timeout is pretty rad. got rid of so many boilerplate loops in my shell scripts. :)