Fix for issue #418

[obourdon]

Fixes #418

Summary of changes:

• added new parameter for allowing binding address+port
• added new test cases
• removed extra space in command line argument specification
• added possibility for default port to be passed as environment variable

Please note that plural was used (bind_addresses and not bind_address) because I have anticipated the fact that we might want to bind on several different addresses + port in the future.

Testing done:

• reran all existing tests successfully
• added tests also successful

@intelsdi-x/snap-maintainers

[IzabellaRaulin]

@obourdon - Travis tests fail because of diff snapd.go and gofmt/snapd.go. Please, make go fmt snapd.go and, just in case, also do that for others go files that have been changed in this commit. Thanks

[lynxbat]

Since it doesn't support multiple addresses yet, we avoid plurals, and for the sake of shorter switches; I would recommend renaming this new switch to --bind-addr.

[lynxbat]

Also - could we instead use an --ip-addr switch and have it include port?

# Just port
snapd --port 8282

# IP and Port to bind to
snapd --ip-addr 127.0.0.1:8282

# If the user provides both we error to make them choose one
snapd --ip-addr 127.0.0.1:8383 --port 8383
> Error: Please use either --ip-addr or --port but not both.

[lynxbat]

The ip-addr example above would also align well to the way to implement multiple listening IPs in golang: https://play.golang.org/p/2IrtRqdGit

[obourdon]

Thanks for all remarks. I'll modify this tomorrow French time ;-) and submit the modified version once tested

[lynxbat]

This is a great PR. Thanks for coming up with this improvement!

[obourdon]

I am almost done with the modifications and tests however I might have "dummy/basic" questions to ask.

• to be completely coherent with both your suggested parameter name (ip-addr) and the already existing port parameters I think it is best to call it addr and not ip-addr because of the possible IP:PORT syntax. However as the -a parameter is already taken by the auto-discover-path I currently left it to be -b (like for previous bind-address[es]) but I do not mind changing it to anything you would consider more appropriate
• I need to have a errors.New("MY MESSAGE") variable which needs to be common between snapd.go and mgmt/rest/server.go. For now, I have included this variable in core/serror/serror.go but I am not convinced that it should really be here. After digging more to see if I could infer something from other parts of the code, I did not manage to find anything which could help me so any suggestion is welcome
• doing the check as requested for removing potential ambiguity between -b IP:PORT1 and -p PORT2 I also need to leave the snapd.go main program "as gracefully as possible" and due to the somewhat old version of codegangsta/cli used here I can not have access to cli.NewExitError(). Again, looking deeper into other part of the code, I could not find anything which could fit this requirement
• My past experiences make me believe that most people are used to the fact that specific value in IP:PORT1 is taking precedence over a more generic -p PORT2 option and that documentation might help making sure people fully understand this but I may be wrong
• last, I have modified the SetAddress function from mgmt/rest/server.go to return an error if for some reason the string passed is not valid

Feel free to comment on all these at will
Thanks again and best regards

[snapbot]

Can one of the admins verify this patch?

[snapbot]

Can one of the admins verify this patch?

[jcooklin]

Let's consider returning errors.New("Invalid address") here.

[obourdon]

OK

[obourdon]

Not sure I understand why the TravisCI jobs were successful for Go 1.5 and not for Go 1.6

[jcooklin]

LGTM