faucetsdn / faucetagent

gNMI agent for faucet configuration
4 stars 2 forks source link

Add config option to set the interface address gNMI should listen on #24

Closed cglewis closed 5 years ago

cglewis commented 5 years ago

Closes #23

lantz commented 5 years ago

Thanks for the PRs btw!!

cglewis commented 5 years ago

@lantz hmm, the rebase somehow included your already merged commit, that's weird.

lantz commented 5 years ago

The tricky bit I think is adding a test for this. The current test exercises the default interface, but we could add another test that listens on IPv4 on another loopback address and/or IPv6 and then verifies that gnmi_capabilities works properly.

lantz commented 5 years ago

@lantz hmm, the rebase somehow included your already merged commit, that's weird.

Yeah, may have to rebase it again since I just merged another PR. That's kind of how it works when things are getting merged in when you are working on something - may have to rebase more than once until all the changes settle.

cglewis commented 5 years ago

I think I have a way to add a test for this - take a look at the updated #26 and if that test looks ok, if it does I'll make a similar one for this one that adds an option to test that arg.

lantz commented 5 years ago

Sounds good; the way you did it in #26 looks good also: add some options and then add some new test methods to EndToEndTest which invoke the test with the appropriate options.

lantz commented 5 years ago

Getting the simple stuff right is often tricky:

What do you think are the right command line options for this? For example:

lantz commented 5 years ago

I think I'm fine with --gnmiaddr including an optional port. The variable name should probably begnmi_addr. Would you like to add a test for this?

cglewis commented 5 years ago

@lantz updated variable names and added to tests.

cglewis commented 5 years ago

Ok, changed.

cglewis commented 5 years ago

@lantz it is rebased against master