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

Inconsistent use of - vs -- for long options in weave script #602

Closed
awh opened this issue Apr 23, 2015 · 25 comments · Fixed by #1084
Closed

Inconsistent use of - vs -- for long options in weave script #602

awh opened this issue Apr 23, 2015 · 25 comments · Fixed by #1084
Assignees
Labels
Milestone

Comments

@awh
Copy link
Contributor

awh commented Apr 23, 2015

For example -password vs --with-dns. -- is the convention for long options on GNU/Linux, we should support it and deprecate the other forms.

@awh awh added the chore label Apr 23, 2015
@awh
Copy link
Contributor Author

awh commented Apr 23, 2015

Docker is making this transition:

$ docker run -privileged
Warning: '-privileged' is deprecated, it will be replaced by '--privileged' soon. See usage.

@squaremo
Copy link
Contributor

squaremo commented May 5, 2015

Another worthwhile convention is separating arguments to a wrapped command with --; e.g.,

weave run 10.2.0.4/24 -- -ti ubuntu

@paulbellamy
Copy link
Contributor

Do we need to maintain backward compatibilty while deprecating (ala docker)? Or are we comfortable with dropping the old syntax?

@awh
Copy link
Contributor Author

awh commented May 22, 2015

#307 might present an opportunity for a clean break...

@awh
Copy link
Contributor Author

awh commented May 22, 2015

#627 (comment)

@paulbellamy
Copy link
Contributor

@awh, agreed, that seems like a good time to fix it.

@rade rade modified the milestone: next May 26, 2015
@rade rade removed this from the next milestone Jun 5, 2015
@squaremo
Copy link
Contributor

I'm not totally sure how to do backwards compatibility, at least with go's built-in flag parser. We can add parallel vars:

    flag.IntVar(&config.ConnLimit, "connlimit", 30, "connection limit (0 for unlimited) (deprecated; please use --conn-limit)")
flag.IntVar(&config.ConnLimit, "conn-limit", 30, "connection limit (0 for unlimited)")

But this will mean usage output gets cluttered, unless we can filter it somehow.

(It also occurs to me that if we cannot have complete backwards compatibility, and we aspire to adhere to SemVer, we will probably need to bump the major version.)

@rade
Copy link
Member

rade commented Jul 1, 2015

I'm not totally sure how to do backwards compatibility, at least with go's built-in flag parser.

I wouldn't mind switching to getopt, which is what the proxy uses.

@squaremo
Copy link
Contributor

squaremo commented Jul 1, 2015

I wouldn't mind switching to getopt, which is what the proxy uses.

Does that have special support for backward-compatibility? (largely a case of conveniently being able to support the old version of an option, while keeping it out of usage)

@rade
Copy link
Member

rade commented Jul 1, 2015

Does that have special support for backward-compatibility?

No idea. But it seems closer, in terms of how options are parsed, to generally accepted usage. E.g. it has sensible semantics for specifying an option more than once (vs. "first one wins).

@squaremo
Copy link
Contributor

squaremo commented Jul 1, 2015

Argh, OK maybe I should revise my idea of what deprecation should do. New rule: deprecated options get output in the usage message, just with "deprecated, please use ....".

By the way, I don't think I can use getopt without breaking backward compatibility, since it won't parse the old, single hyphen versions of the options. Piiiiiike!

@rade
Copy link
Member

rade commented Jul 1, 2015

I'd be ok with a non-backward-compatible change.

@bboreham
Copy link
Contributor

bboreham commented Jul 2, 2015

We could add some code in the weave script to translate old options to new ones and warn, to ease the transition for existing users.

@squaremo
Copy link
Contributor

squaremo commented Jul 2, 2015

We could add some code in the weave script to translate old options to new ones and warn, for existing users.

Easier said than done, now that I think about it. For one thing, the options are potentially context-sensitive -- it's possible (maybe?) that option-looking things appear as values.

@squaremo
Copy link
Contributor

squaremo commented Jul 2, 2015

Oh and the other thing -- it couples the script to all executable options, even the ones it would splat through without looking at them.

Mainly for that reason, I'm now looking at doing the deprecation in the exes themselves instead.

@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

I am now thinking I should anticipate the merge of weaveDNS and weaver by prefixing weaveDNS options with --dns-

@rade
Copy link
Member

rade commented Jul 3, 2015

I am now thinking I should anticipate the merge of weaveDNS and weaver by prefixing weaveDNS options with --dns-

Most of the DNS options will go away. It's up to #826 to deal with the options.

@rade rade added this to the current milestone Jul 3, 2015
@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

It's up to #826 to deal with the options.

Sure; but I presume some will still remain (like httpport), and I'd rather not change their names twice. Although it is true that merging weave and weaveDNS will represent a bigger change, it's friendlier if people don't have to go back and change all the options too. I'm only changing those that have a chance of surviving.

(Not that I expect many of them would be in use, to be honest)

@rade
Copy link
Member

rade commented Jul 3, 2015

httpport

will be the same as the weave http port.

There are currently exactly three options pertaining to DNS in the gossip-based implementation, and one of those (nameserver) should go.

@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

There are currently exactly three options

What about

  • --fallback (--dns-fallback)
  • --maxanswers (--dns-max-answers)
  • --watch (?)
  • --ttl
  • --negttl
  • --udpbuf

which are still arguably relevant?
?

@rade
Copy link
Member

rade commented Jul 3, 2015

Well, that's really up to Tom. I suggest you don't try to second-guess what gossipDNS will end up looking like.

@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

I suggest you don't try to second-guess what gossipDNS will end up looking like.

You mean phonetreeDNS

@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

It's up to #826 to deal with the options.

Are you saying I should leave weaveDNS entirely alone and assume the new implementation will conform to the changes here? I've made a note in #826 suggesting that last.

@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

Oh and the other thing -- it couples the script to all executable options, even the ones it would splat through without looking at them.

Mainly for that reason, I'm now looking at doing the deprecation in the exes themselves instead.

Of course, by doing it in the exes, the deprecation notices go to the logs, meaning almost no-one will see them. So perhaps I'll put deprecation notices in for the arguments that are explicitly mentioned in the script usage.

@rade
Copy link
Member

rade commented Jul 4, 2015

From #1035 (comment)

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.

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).

Looking at the complications we are running into, and the time it's taken so far, I suggest we skip the deprecation.

@rade rade modified the milestones: current, 1.1.0 Jul 13, 2015
marccarre added a commit that referenced this issue Mar 1, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Mar 7, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Mar 7, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Mar 8, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Mar 9, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Mar 15, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Apr 3, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Apr 4, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Apr 7, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
brb pushed a commit that referenced this issue Apr 7, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
marccarre added a commit that referenced this issue Apr 11, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
bboreham pushed a commit that referenced this issue Apr 12, 2017
Since 1.12.0, `docker inspect -f "{{ .NetworkSettings.Networks.<network>.Aliases }}"` also returns the container's short ID.
See also: https://github.com/docker/docker/blob/master/CHANGELOG.md
> 1.12.0 (2016-07-28)
> Networking
> Add container's short-id as default network alias #21901

Upgrading `docker/docker-py` to 1.9.0-rc2 effectively relaxes test assertions for:

- `tests/integration/network_test.py#test_connect_with_aliases`
- `tests/integration/network_test.py#test_create_with_aliases`

which do something equivalent to:

```
$ docker network create net1
$ docker run --name c1 -d busybox top
$ docker network connect --alias foo --alias bar net1 c1
$ docker inspect -f "{{ .NetworkSettings.Networks.net1.Aliases }}" c1
[foo bar e52bf50e5ee7]
```

allowing us to be consistent with the above change.

In 1.8.1, the below fails as the container's short ID would be present:

```
        self.assertEqual(
            container_data['NetworkSettings']['Networks'][net_name]['Aliases'],
            ['foo', 'bar'])
```

In 1.9.0-rc2, the below passes as the container's short ID is ignored:

```
        aliases = (
            container_data['NetworkSettings']['Networks'][net_name]['Aliases']
        )
        assert 'foo' in aliases
        assert 'bar' in aliases
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants