[Enhancement] Api port #75
Conversation
--api-port only takes the port as argument. This patch modifies it to take a string argument, in the form of host:port.
Now that --api-port takes a string argument, add a parser function to handle error checking for this argument.
Pass the apiPort sturct directly via ClusterSpec.
To allow some flexibilities in how user specifies the --api-port argument. In case the 'host' is an string, We will try to convert it into an IP address for port mapping. IP address is also allowed.
cli/commands.go
Outdated
|
|
||
| // When the 'host' is not provided by --api-port, try to fill it using Docker Machine's IP address. | ||
| if apiPort.Host == "" { | ||
| if apiPort.Host, err = getDockerMachineIp(); apiPort.Host != "" || err != nil { |
There was a problem hiding this comment.
What's the reason for having apiPort.Host != "" in this condiftional? There's no action if this is true, right?
There was a problem hiding this comment.
You are right. I have refactored code and forgot to remove this check. The line below should also be removed. Will fix before merge. Thanks for catching it!
| @@ -20,6 +20,20 @@ import ( | |||
| "github.com/docker/docker/client" | |||
| ) | |||
|
|
|||
| type ClusterSpec struct { | |||
There was a problem hiding this comment.
Maybe at some point we can add a new level here for server and agents, if we add more options?
E.g. you would call the server args via clusterSpec["server"].Args
Achieved by a new struct for ServerSpec and one for AgentSpec and embedding them into the ClusterSpec struct.
WDYT?
There was a problem hiding this comment.
I see your view point and agree that we should consider this approach down the road.
Right now, both server and agent share a lot of information, the clusterSpec is a simple union of serverSpec and agentSpec. It seems to be a good abstraction. At the same time, I do not oppose the way you have suggested.
Based on review comments from @iwilltry42
Add support for --api-port to take [host]:port as argument.