tiltfile: custom host in port forwards #2382
Conversation
|
The way to specify host is with a string like '0.0.0.0:3000:3000', which I believe should be standard. |
| } | ||
| return portForward{local: int(n)}, nil | ||
| } | ||
|
|
||
| const ipReStr = `^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$` | ||
| const hostnameReStr = `^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$` |
There was a problem hiding this comment.
where did these regexps come from?
There was a problem hiding this comment.
Not sure what exactly you're asking. These are pretty standard regexps for IPs and hostnames, not sure if it's possible to pinpoint their exact source, cause they are pretty standard, based on relevant RFCs. Here are examples of where they can be encountered online:
IP: http://ipregex.com
Hostname: https://www.regextester.com/23
I've been using them for a while and I don't remember where I took them from, but again, they are pretty standard, so probably shouldn't matter.
| func stringToPortForward(s starlark.String) (portForward, error) { | ||
| parts := strings.SplitN(string(s), ":", 2) | ||
| local, err := strconv.Atoi(parts[0]) | ||
| parts := strings.SplitN(string(s), ":", 3) |
There was a problem hiding this comment.
where did this syntax come from?
it's probably ok, but seems different from the kubectl port-forward syntax (which has a separate --address flag). https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/portforward/portforward.go#L87
i guess the advantage of the separate address flag is that you can specify multiple
There was a problem hiding this comment.
Just as with the previous discussion, not sure what the exact origin of this syntax is, but it seems to be the most commonly used way to specify hostname for port forwarding, one example is OpenSSH.
You're right, it doesn't allow for multiple addresses, so if we need to support that, we should probably change syntax to something else. That being said, in my experience there's barely ever a need to bind to multiple addresses, you usually want it to either be accessible on a specific network, or accessible on every network (which can be done by specifying 0.0.0.0 as the address).
| LocalPort int | ||
|
|
||
| // Optional host to bind to on the current machine (localhost by default) | ||
| Host *string |
There was a problem hiding this comment.
what's the purpose of making it a pointer? would we want to treat the nil value differently than the empty string?
There was a problem hiding this comment.
Two thoughts:
A) Even if a piece of information is not currently used, but might be used in the future, and capturing it doesn't require any overhead, imo it's better to capture it now than to refactor later.
B) Right now if the value is nil we do port forwarding without addresses, and if the value is empty string we simply pass it to NewOnAddress, which I'm assuming explicitly fails with some error (tho I haven't tested it tbh). So this signature makes it slightly easier to catch bugs in case hostname is meant to be supplied, but ended up being empty string. This is probably rather opinionated.
|
ok, sgtm. Two minor points:
thanks for the addition! |
Thank you for your prompt review and merge! |
Fixes #2013.
If this gets merged, I can create a PR to update the docs as well.