Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

role of iprange not clear #1035

Closed
rade opened this issue Jun 28, 2015 · 16 comments · Fixed by #1084
Closed

role of iprange not clear #1035

rade opened this issue Jun 28, 2015 · 16 comments · Fixed by #1084
Assignees
Milestone

Comments

@rade
Copy link
Member

rade commented Jun 28, 2015

A user reports "My worry about using the -iprange was that maybe it would restrict the ip range I could use weave with."

Looking at our docs, it is indeed not absolutely clear that iprange is used for automatic allocation only.

@squaremo
Copy link
Contributor

I'm sure there is a long and storied history, but on the surface of it, the names of those arguments do not make it clear what they do. We could fix the documentation, or fix the names, or both. (If we fix the names, we'll have to keep the old ones for compatibility I suppose)

@rade
Copy link
Member Author

rade commented Jun 29, 2015

Lets fix the names. What have you got in mind?

@squaremo
Copy link
Contributor

What have you got in mind?

Discussion on <group chat du jour> brought up --ipam-range and --ipam-default-subnet. I'll leave in support for -iprange and -ipsubnet, with a deprecation notice.

@rade
Copy link
Member Author

rade commented Jun 29, 2015

This naming scheme is inconsistent with what we currently have for router options. We don't use dashes in options, and we use - instead of --.

@squaremo
Copy link
Contributor

Those are both things I dislike! And -ipamrange and -ipamdefaultsubnet are difficult to read.

@rade
Copy link
Member Author

rade commented Jun 29, 2015

Those are both things I dislike!

Fine, so fix them.

Also note that ipam is not a term we generally use in our docs. #1040

@squaremo
Copy link
Contributor

note that ipam is not a term we generally use in our docs.

ipalloc could be used in place of ipam. A bit longer, but more obvious if we don't actually refer to IPAM anywhere.

Those are both things I dislike!

Fine, so fix them.

As a prerequisite to doing this?

@rade
Copy link
Member Author

rade commented Jun 29, 2015

As a prerequisite to doing this?

Yes. Or stick to the current scheme (which would be my recommendation; and raise a separate issue for revamping the flag parsing).

@squaremo
Copy link
Contributor

No-one in good conscience could add an option -ipallocdefaultsubnet. Nicely punted into the long grass, there.

@rade
Copy link
Member Author

rade commented Jun 29, 2015

We already have initpeercount.

@squaremo
Copy link
Contributor

Yeah that's not ideal either really

@squaremo
Copy link
Contributor

We already have initpeercount.

.. and --with-dns, --without-dns, --replace, --no-default-ipam, ....

@rade
Copy link
Member Author

rade commented Jun 29, 2015

.. and --with-dns, --without-dns, --replace, --no-default-ipam, ....

None of those are launch options. And they are never mixed with other options that follow a different scheme.

@squaremo
Copy link
Contributor

Oh right, it's one of those "except when it isn't" rules.

@rade rade modified the milestone: current Jul 2, 2015
@rade
Copy link
Member Author

rade commented Jul 3, 2015

I have been thinking about the backward compatibility story... I reckon what users are most concerned about isn't substitutability, but interoperability - being able to run new and old in the same network, and thus being able to roll out and upgrade gradually. That requires protocol compatibility, not script compatibility.

Hence renaming of commands & options is actually fine, though obviously if it is easy to retain the old ones in parallel then we should do so (but advise users in the release notes that they should really update their scripts, etc, since we won't retain the old version indefinitely).

@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

@rade Should we cross-post that to #602?

@rade rade removed this from the current milestone Jul 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants