Construct Compose provider URLs through container inspection #80
Conversation
This commit switches from hardcoded URLs that require a special host definition to dynamic, inspection-based URLs. There's just a smidgen of skullduggery in ensureStandaloneRunnerAvailable, but I can clean it up in the next PR (which will allow fully dynamic Docker CE and cloud ports). Signed-off-by: Jacob Howard <jacob.howard@docker.com>
| if port.IP == "127.0.0.1" { | ||
| result.hostPort = port.PublicPort | ||
| } else { |
There was a problem hiding this comment.
nit: should we use net.ParseIP(port.IP).IsLoopback() instead ?
There was a problem hiding this comment.
I think not neccesarily, it seems to me the idea is to match the hardcoded IP here
model-cli/pkg/standalone/containers.go
Line 108 in 0d20212
There was a problem hiding this comment.
I think we want to leave it matching the hardcoded because in theory we may be talking to a remote Docker CE instance where IsLoopback() might behave differently (though the current impl looks consistent). I'll keep this in the back pocket though. net.ParseIP might be useful for identifying IP classes in general.
| // inspectStandaloneRunner inspects a standalone runner container and extracts | ||
| // its configuration. | ||
| func inspectStandaloneRunner(container container.Summary) *standaloneRunner { | ||
| result := &standaloneRunner{} |
There was a problem hiding this comment.
question: would adding a label with the <host>:<port> where the DMR is running to the container on creation and then reading it here be more robust?
There was a problem hiding this comment.
I want to avoid that because I think we may want a fully dynamic binding mode for standalone runners where they bind on :0 and we inspect the port once running.
| if port.IP == "127.0.0.1" { | ||
| result.hostPort = port.PublicPort | ||
| } else { |
There was a problem hiding this comment.
I think not neccesarily, it seems to me the idea is to match the hardcoded IP here
model-cli/pkg/standalone/containers.go
Line 108 in 0d20212
| // standaloneRunner encodes the standalone runner configuration, if one exists. | ||
| type standaloneRunner struct { | ||
| // hostPort is the port that the runner is listening to on the host. | ||
| hostPort uint16 |
There was a problem hiding this comment.
This doesn't seem to be used.
There was a problem hiding this comment.
I'm planning to use it in a future PR where we make the listener port configurable. If I don't manage, I'll remove it, scout's honor ✋
Signed-off-by: Jacob Howard <jacob.howard@docker.com>
This PR switches from hardcoded URLs that require a special host definition to dynamic, inspection-based URLs.
There's just a smidgen of skullduggery in
ensureStandaloneRunnerAvailable, but I can clean it up in the next PR (which will allow fully dynamic Docker CE and cloud ports).