make weave port configurable via WEAVE_PORT #551
Conversation
We cannot just leave that to the `-port` parameter since that only controls the container internal port weave is listening on (and uses as the default destination port for outbound connections). Also, we need the port for connection tracking cleanup during `stop` and `reset`. So we make setting `WEAVE_PORT` the only documented mechanism for overriding the port, and let `-port` set the container internal port only (and the script pays attentiont to that, so it can establish the correct port mapping).
|
@dpw review please |
|
Other than the comment about |
|
The general rule I've tried to follow is that |
Ok. The |
|
any more confusing than |
|
The confusingness of |
|
It is pertinent because if we go with |
|
I'm not saying it has to be `WEAVE_PORT` throughout. That does seem like a
good solution to me, but it's not the only solution to the confusion I see,
so you are welcome to solve it a different way.
The confusion is because we now have two variables, `WEAVE_PORT` and
`PORT`, which both carry the, um, weave port, but with a subtly different
emphasis to their use. This worsens a the preexisting issue: While `PORT`
might have been a fine variable name when there was only one port number in
the script, it is not so good when there is also `HTTP_PORT` etc. But even
if `HTTP_PORT` did not exsit, the lack of a hint at the distinction between
`PORT` and `WEAVE_PORT` would still be a problem.
I don't have a good idea for a name (hence the attraction of re-using
`WEAVE_PORT`). But sometimes when I have a difficulty naming something, I
find a useful approach is to start by writing a comment introducing its
purpose, and then to boil that comment down until it can be captured in the
name, so that the comment can be dispensed with.
|
|
PROTOCOL_PORT? PEER_PORT? |
|
You appear to be having trouble getting this bikeshed painted ;) GOSSIP_PORT? |
The port is used for more than gossip. |
These do not seem to hint at the purpose of the variable as it contrasts with the purpose of |
|
I will just leave it as PORT then. |
|
I'm not raising this point frivolously, I genuinely think the distinction between WEAVE_PORT and PORT is not obvious. And that given state of the weave script, we need an abundance of caution when introducing changes that tend to make it worse. |
|
I don't think it's a frivolous point. But I cannot come up with a better name. It's not worth holding this up for that. |
|
I was clear what I thought the best way to address this was up front (I.e. conflate WEAVE_PORT and PORT). You didn't like that suggestion, because some other thing should be changed for consistency. That might be a valid point, but it's not one I raised, and not one I would have held things up for. And you could have made the wider change for consistency if you believed that proper - it would not have been time consuming. As for changing the name of PORT, I don't think an earnest attempt to find a better |
|
(Cont) name would take more than five or ten minutes. In short, I don't think I am the one holding this up, except by providing feedback, and guidance on what would or would not satisfy that feedback. If you want someone to rubber stamp your PRs without giving them careful consideration, find someone else, you know that's not my style. |
I didn't suggest you were. To be clear, I am not asking you to merge this PR.
I don't mind the careful consideration. Quite the opposite. But sometimes there will be disagreement about a particular piece of feedback. And sometimes it's not worth holding up a PR until that is resolved. I'll let this one sit until tomorrow. If somebody comes up with a mutually acceptable name for the var then I'll make that change. Otherwise I'll merge this as is. |
Ah, but who gets to make the call - there's the rub. If a developer can unilaterally override, then that leads to a low standard of review (because all developers are reluctant to change work they have invested in, or to do further work they regard as unnecessary; and because reviewers have a disincentive to be diligent if their diligence may be disregarded at a whim).
We both know how that will go, so this seems like a pretense. |
|
I disagree, but let's not have that meta discussion in this issue, please. |
|
The flag help for |
|
My opinion on this one may be by the by, but if PORT became ROUTER_PORT, then WEAVE_PORT could become WEAVE_ROUTER_PORT on exactly the same basis, and we'd be more or less back where we started I am not suggesting renaming for the sake of renaming, with a name that is merely different from WEAVE_PORT and longer than PORT, but rather I'd like a name that accounts for the specific purpose of that variable. Yes, it's a port number, yes it's the router port number, but why does it exist in addition to WEAVE_PORT? |
|
I think the problem here stems from the fact that PORT is used for several things - e.g. both the control plane traffic over TCP and the forwarding path over UDP, both of which are distinct protocols proprietary to the weave router. As a consequence it's quite hard to give it a pithy name! Actually I agree with your suggestion to do |
make weave port configurable via WEAVE_PORT
|
@awh please open a separate issue if you want to go on a var renaming spree. As for your latest suggestion. See #551 (comment)... The |
This is a follow on of #534.