move port splitting to common place; add to node resource location #7073
Conversation
|
To unblock the UI PR. |
|
@lavalamp: I'll try now. |
|
@lavalamp this seems to work as expected. I can now do requests like: |
| // | ||
| // Port is returned as a string, and it is not required to be numeric (could be | ||
| // used for a named port, for example). | ||
| func SplitPort(id string) (name, port string, valid bool) { |
There was a problem hiding this comment.
given that in all cases you test valid and then return an error, perhaps just return an error instead of the 'valid' bool?
There was a problem hiding this comment.
The error is different in all three places, though... I also don't think util can return an api error (dependency goes the other direction), and if I return a regular error, then it has to get wrapped in all three places anyway.
|
Basically LGTM, small comment on a refactor that I think makes the code cleaner. |
|
@brendanburns can this be merged as-is as I think @lavalamp is correct unless I'm missing something. |
|
I agree with @lavalamp. |
piosz
added
the
lgtm
move port splitting to common place; add to node resource location
| "strings" | ||
| ) | ||
|
|
||
| // Takes a string of the form "name:port" or "name". |
There was a problem hiding this comment.
This is very similar to but different from http://golang.org/pkg/net/#SplitHostPort - is it worthwhile, really? or should we thunk down to that when we can?
There was a problem hiding this comment.
Yeah, we can investigate whether we can just use that. I wanted to at least get these three cases consistent, though.
No description provided.