Improve argument parsing, and allow establishment of route(s) via the new container interface #27
Conversation
|
Hi! First, I'd like to apologize — the notification email for your pull request got buried under a ton of other things, and I'm only looking at your changes now. 1 month, phew, that sucks. Really sorry. I'm willing to merge the new argument parsing, and the route option. However, do you think the verbose mode could be left to #11? Also, I'm OK with changing the various conditionals and quoting, put please-please can you put that in a different PR? (That will make it much easier for me to review.) In any case, thanks a lot for your contribution, and I hope that you're not too unhappy about my slow answer :-( Best, |
…ia the bridge interface
o Document the new --route ... option o Parse arguments a with more flexibilty, and full backward compatibilty o Clean up conditionals to use if ...; then instead of short-circuit o Quote variable tests o Log a play-by-play if -v|--verbose option given
o Shamelessly copy jpetazzo#41, and modify it to more simply fall-back to docker if cgroups lookup doesn't work. o Ensure we properly test for failures when seeking data from cgroups or docker, to avoid silent failure due to the "set -e" o Consolidate all indentation to use spaces instead of inconsistent tabs
|
I've rebased this pull request on to current master, and augmented it. |
o Using short-circuit (( <variable> )) && ... will evaluate falsey if <variable> contains nothing, which is useful; however, this is considered an "error" by set -e, causing the program to exit. o Add --trace|-x option to enable bash set -x command tracing
o Make optional, to avoid issue when not available in the container; we cannot check for availability from outside! o Note what version of arping is required; there are two (incompatible)
|
Corrected some further issues with arping; checking for the availability of the command is useless, because the container (usually) will have a different set of commands available wtihin it (and we cannot check). So, make it optional with -a|--arp, and warn on command failure. Also, since there are (at least) two versions of arping in widespread use, made a documentation note in the code to indicate what version is necessary, for those that are interested. Also added -x|--trace to enable bash command tracing; very useful for working out failures. Incidentally, "set -e" is widely considered to be quite fragile, but I think I've rooted out most of the unrecognized failures due to its use... |
o Default to 3 tries o Log arping output on failure
|
Sorry again — since your PR is bigger than others, it always went to the bottom of the stack, as I don't get a lot of time to work on pipework :-/ Some of those changes have been merged separately already. If you are still willing to work on this PR, could you break it down in multiple parts? Specifically:
Note regarding arping: with |
The parsing of arguments has been improved, adding support for long- and short-form options.
New -v|--verbose option allows pipework to print a play-by-play of argument parsing and what it is doing, to assist in diagnosis when things go unexpectedly.
The new -r|--route option allows specifying one or more new routes in the container using the new interface. This is useful, for example, to set up multicast to use the new interface, eg. --route 224.0.0.0/4
Also cleaned up some of the conditionals and variable quoting.