One more - shows docker inspect showing the mount correctly.

I don't have any concerns other than I want to understand what's going on.
Will finish reviewing tonight.
On Thu, Oct 15, 2015 at 4:43 PM David Calavera notifications@github.com
wrote:

Okay, I think we should move this forward, but I know @tiborvass
https://github.com/tiborvass has some concerns. I don't think we can do
much about the server side vs client side validation.

@jhowardmsft https://github.com/jhowardmsft do we need to modify any
documentation that talks about volumes to say that windows also supports
volumes now?

—
Reply to this email directly or view it on GitHub
#16433 (comment).

@tiborvass Anything else found?

ping @tiborvass @cpuguy83 @calavera

With one LGTM, AFAI can tell, there are two things outstanding on this. One: Tibor/Brian/David agreeing on a name should it require revising for -this- PR (it can be changed in a subsequent one as it's not going to hit 1.9 anyway). The name decision has been outstanding since Thursday and has stalled. Second, @tiborvass finishing his review.

Can I get a status update as to where we are. What else do I need to do? This is a critical TP4 PR, for all parties involved - docker inc have indicated on multiple occasions now that this is one of the most highly requested features for docker on Windows. From a Microsoft perspective, this is necessary to support multiple TP4 scenarios.

@tiborvass Updated following our IRC conversation yesterday so that a volume with a mode, eg /foo:rw (Unix) or c:\foo:rw (Windows) fails. [This was left over incorrectly from the split of the combined device and volume parsing on the client side]

Ugh, go-lint caught me out. Updated.

Phew!!!!!!! OK, so added a second commit based on Tibor's most excellent work. Still running integration cli locally to verify, but believe it should pass (it did yesterday, shouldn't have changed). What is now fixed from the commit I pointed you guys at yesterday is the rejigging of unit tests, which do pass locally on both Windows and Linux.

So, gone is the extra field in Config. Yay!

@calavera @tiborvass @cpuguy83 PTAL, and if looks good, I'll squash the commits. Keeping them as two for now to make your review easier.

Rebased, but squashed the commits. You can see the config struct parameter removal change independently at https://github.com/microsoft/docker/tree/jjh/tiborupdate

LGTM

@calavera You're not supposed to say that - I had tomorrow blocked off to address comments on this. What am I going to do instead lol! ;)

LOL

An invalid -v bind is yielding an error: Invalid bind mount spec "/foo:/bar:baz": volumeinvalidmode: invalid mode for volumes-from: baz

Looks like the volumeinvalidmode bit is incorrect.

Seems there are two similar errcodes:

    // ErrorCodeVolumeInvalidMode is generated when we the mode of a volume
    // mount is invalid.
    ErrorCodeVolumeInvalidMode = errcode.Register(errGroup, errcode.ErrorDescriptor{
        Value:          "VOLUMEINVALIDMODE",
        Message:        "invalid mode for volumes-from: %s",
        Description:    "An invalid 'mode' was specified in the mount request",
        HTTPStatusCode: http.StatusInternalServerError,
    })

    // ErrorCodeVolumeMode is generated when 'mode' for a volume
    // isn't a valid.
    ErrorCodeVolumeMode = errcode.Register(errGroup, errcode.ErrorDescriptor{
        Value:          "VOLUMEMODE",
        Message:        "invalid mode for volumes-from: %s",
        Description:    "An invalid 'mode' path was specified in the mount request",
        HTTPStatusCode: http.StatusInternalServerError,
    })

Neither seem to have the correct message, one can probably be eliminated, not sure....

Interesting. I don't recall changing that, but maybe I did. Should be an easy fix.

Duplicate/Incorrect error fix will be in the next push.

@cpuguy83 Pushed updates for error codes and moving unit tests out of ext_endpoint. Looking at the name normalise. Might ping you on IRC...

@tiborvass - Looks like all the comments are resolved, and I don't think there's anything left outstanding now from @cpuguy83 and @calavera. Yay! 👍

Is there anything else from your perspective left to change, or are we ready (post 1.9 rc2) to move this to a merge status?

Thanks!!!

LGTM, minor nit about error message on the client, but can be dealt with separately, given the time, effort and impact this PR has.

Merging since docs will be dealt separately as tracked in #15597

\o/