Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOCKER_VERSION and docker-version havn't been implemented. #8673

Merged
merged 1 commit into from Oct 23, 2014

Conversation

SvenDowideit
Copy link
Contributor

So far, it looks like the declarations are not used, and so its safer not to
confuse people into thinking they do something.

Docker-DCO-1.1-Signed-off-by: Sven Dowideit SvenDowideit@docker.com (github: SvenDowideit)

@tianon when was the last time someone made sure that our main Dockerfile worked on Docker 0.6.1, and are we really intending to send that message?

@jfrazelle are you suggesting that your Dockerfiles cannot be used with older Docker daemons? - and which is it :) (I know where these came from)

@me - seriously, do you really thing Docker 0.3 is something we want to bring up in the docs?

On a serious note - is this INSTRUCTION being worked on, in which case I'll reduce the scope to the documentation examples, where its distracting, or can we move on?

@crosbymichael feel free to yell at me about busting the cache - maybe we can use the golang:1.3-onbuild image - its only 1.5GB big >:+

So far, it looks like the declarations are not used, and so its safer not to
confuse people into thinking they do something.

Docker-DCO-1.1-Signed-off-by: Sven Dowideit <SvenDowideit@docker.com> (github: SvenDowideit)
@LK4D4
Copy link
Contributor

LK4D4 commented Oct 21, 2014

LGTM

@crosbymichael
Copy link
Contributor

@SvenDowideit no yelling, this is worth the cache bust

@tianon
Copy link
Member

tianon commented Oct 21, 2014

+1000 LGTM

I always hated this line. Also, the builder ignores unknown instructions, so in theory, this shouldn't be a cache-bust.

@wking
Copy link

wking commented Oct 22, 2014

On Tue, Oct 21, 2014 at 04:37:33PM -0700, Tianon Gravi wrote:

Also, the builder ignores unknown instructions, so in theory, this
shouldn't be a cache-bust.

Is this really a good idea? It's going to make breakage like the ADD
→ COPY bit more confusing than and ideal:

“unrecognized command 'COPY', please upgrade your Docker installation”

(see #7471, but I'm not sure what the actual error would have been).

Personally, I think specifying a minimum Docker(file) version is a
good idea (see also #4907), so when I dust off a 1.2.0 Dockerfile in
five years and try to build it with Docker 5.3.7 I know I get to keep
the pieces (and I know that I only need to read the release notes for
2.0.0, 3.0.0, 4.0.0, and 5.0.0 to see what changes I need to make).
It also makes life easy for a hypothetical Dockerfile-update script
(like 2to3, see #4907). I'm fine with rolling releases for something
loose like HTML, but when the goal is stable, reproducible builds, it
seems like more caution would be useful ;).

Having explicit Dockerfile versions would also make it possible for
the Trusted Build infrastructure to slot in an older builder if the
Dockerfile in question wasn't compatible with the latest builder.
That's going to make life a lot easier when you get the 2.0 Dockerfile
format that breaks compatibility with all the existing Dockerfiles.
On the other hand, maybe there will never be a 2.0 Dockerfile format,
and all changes will be backwards compatible forever (like the Linux
kernel 1)?

This is a drive-by comment, so apologies if I'm just missing context
;).

@SvenDowideit
Copy link
Contributor Author

@jamtur01 @fredlf :)

@wking my POV is that yes, it would be nice, but until it is, there's nothing useful about having these strings in here, rotting away.

@jamtur01
Copy link
Contributor

LGTM

@wking
Copy link

wking commented Oct 22, 2014

On Tue, Oct 21, 2014 at 09:11:30PM -0700, Sven Dowideit wrote:

my POV is that yes, it would be nice, but until it is, there's
nothing useful about having these strings in here, rotting away.

Ah. That makes sense to me :).

@thaJeztah
Copy link
Member

@wking yes, I'd love to see it supported as well, but if it's basically just clutter at this moment, it's best to remove it (for now), otherwise people have false expectations that it's actually doing something. So fwiw, LGTM

@jessfraz
Copy link
Contributor

LGTM I just put it bc the other ones have it

@jessfraz
Copy link
Contributor

we should definitely be using our own best practices to set an example for others, which reminds me I wanted to cleanup the apt-get's in the docs Dockerfile ;)

@fredlf
Copy link
Contributor

fredlf commented Oct 22, 2014

LGTM

@SvenDowideit
Copy link
Contributor Author

1006 LGTM's (if i generously interpret @crosbymichael 's comment as one :) so I'm just going to merge

SvenDowideit added a commit that referenced this pull request Oct 23, 2014
DOCKER_VERSION and docker-version havn't been implemented.
@SvenDowideit SvenDowideit merged commit bb3e331 into moby:master Oct 23, 2014
SvenDowideit added a commit to SvenDowideit/docker that referenced this pull request Nov 6, 2014
DOCKER_VERSION and docker-version havn't been implemented.
(cherry picked from commit bb3e331)
@SvenDowideit SvenDowideit deleted the whats-docker-version branch April 26, 2015 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants