Skip to content

Conversation

@shin-
Copy link

@shin- shin- commented Jun 28, 2016

Fixes #3637

@shin- shin- force-pushed the 3637-link-local-ips branch 4 times, most recently from 8d25c33 to 20a7813 Compare July 1, 2016 21:41
version = V2_0

if version != V2_0:
if version not in (V2_0, V2_1):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: maybe if version == V1 would be more correct here, since it's the only version that would not be supported (now and in the future).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we want to raise an error here if the user inputs a non-yet released version (like "2.5" or "3"), so I think the test as it is is more correct.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. I forgot this could still be any string.

@shin- shin- force-pushed the 3637-link-local-ips branch 2 times, most recently from d2d395f to db3059b Compare July 6, 2016 20:17
@aanand aanand added this to the 1.9.0 milestone Jul 21, 2016
@shin- shin- force-pushed the 3637-link-local-ips branch from db3059b to fc58728 Compare July 25, 2016 22:22
@shin-
Copy link
Author

shin- commented Jul 25, 2016

Rebased, included docker/docker-py#1139

Signed-off-by: Joffrey F <joffrey@docker.com>
Signed-off-by: Joffrey F <joffrey@docker.com>
@shin- shin- force-pushed the 3637-link-local-ips branch from fc58728 to 66b395d Compare September 15, 2016 21:55
@shin-
Copy link
Author

shin- commented Sep 15, 2016

Rebased!

Copy link

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple small things I missed before, otherwise LGTM

I think pretty soon we'll want to add a Version object to make it easier to compare versions, but I don't think we need it just yet.

I also wonder if we could maintain the jsonschemas by having the 2.1 schema be just an addition to the 2.0 schema (maybe by including the 2.0 schema). I don't think we need to make that change in this PR, but we could look at it later.


version = config.version
if version not in (V2_0, V2_1):
version = V2_0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably default to latest (V2_1) ?


#### link_local_ips

> [Version 2.1 file format](#version-2.1) only.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll be easier to maintain these docs if we word this as: "Added in Version 2.1 ..." instead of "only".

That way we don;t have to change the text when we add version 2.2 or 3

#### link_local_ips

> [Version 2.1 file format](#version-2.1) only.
> [Added in version 2.1](#version-2.1).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this probably needs to be "file format version 2.1" or something like that, so it's not confused with the Compose version, right?

Minor docs fix

Signed-off-by: Joffrey F <joffrey@docker.com>
@shin- shin- force-pushed the 3637-link-local-ips branch from b3b8a44 to 3342418 Compare September 20, 2016 18:47
@shin- shin- merged commit f65f89a into docker:master Sep 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants