Closed MelanieT closed 8 years ago
Hi ! Thanks for your pull request. However I already implemented and released the com.dnsdock.ip_addr label It seems the only difference is the Regex support. I'll modify it if you want.
The two are orthogonal. Your ip_addr label, of which I am aware, can only be used to assign a fixed address as an override. For that, the IP address needs to be known before starting the container. Docker assigns IP addresses on startup, so there is really only very limited use to the ip_addr tag. The prefix tag I implemented allows to use Docker's assigned address, but only specify the network the published IP should be on. dnsdock will then choose the IP on the requested network after Docker has assigned it. This is needed because overlay networks have no connectivity with the host and dnsdock defaults to publishing the overlay network's address because Docker assigns that to eth0. So, this feature is definitely needed in this form, the ip_addr tag doesn't do the same thing.
Ok I understand the difference The two proposals are not so orthogonal in the way that conflicts may happen when the two are specified. I'll give priority to ip_adress as it demonstrates a real will of the user to force a specific value. I'll integrate the new option however I'll change the name 'prefix' to 'selector' or filter as it fits more the functionality
Well, personally I believe prefix is the right name to use because the term is used in networking everywhere to denote a partial IP address beginning at the front. People would recognize prefix as what it is, while selector isn't used in networking at all and filter would only be appropriate if there were a possibility to use rules, like regex, and the result could be more than one IP, I know it's semantics but I believe that usability is improved when using a term people know as opposed to one they never heard in that context. The most clear would be "network_prefix", actually.
I believe the way it is coded now already does give ip_address priority because it is applied later in the flow.
Your arguments about prefix are solid, I kept the name as it is. I have done some global refactoring to the code for release 1.16.0 and released it.
I've read through the resulting code and I have a suggestion to make:
The normal user who this project targets will not understand golang, so "the source is the documentation" won't work well. I would suggest adding a message that tells the user if a prefix isn't matched. If the user gives a prefix it is reasonable to believe that the user expects the network matching that prefix to be chosen. They may not look for the issue in their own setup but blame dnsdock because there isn't anything in the log they can associate with a failure to detect the given prefix. They just get the message "service ignored". A possible wording would be "The prefix
Goog point. I've changed this and pushed it on master. It will be in the next release.
Goog point. I've changed this and pushed it on master. It will be in the next release.