36395 mount print

[agawish]

fixes #36395

- What I did

1. Removed errBindNotExist variable from validate.go as it is not used anymore.
2. Updated linux_parser.go and windows_parser.go to display the Source directory in the error message.
3. Updated validate_test.go with the new error message.

- How I did it
Followed the contribution guide to build the docker environment and made changes to the source code.

- How to verify it
Run go test -timeout 30s -run ^TestValidateMount$

- Description for the changelog
It was a quick change to include the source directory path in the error message displayed to the user, files effected are 4 files.

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

Thanks @agawish - looks like there's a test that needs to be updated;

03:01:38 --- FAIL: TestParseMountRaw (0.02s)
03:01:38 	volume_test.go:313: ParseMountRaw(`"c:\\notexist:d:"`) error should contain "source path does not exist", got invalid volume specification: 'c:\notexist:d:': invalid mount config for type "bind": bind mount source path: 'c:\notexist' does not exist
03:01:38 	volume_test.go:313: ParseMountRaw(`"c:\\notexist:/foo"`) error should contain "source path does not exist", got invalid volume specification:

[agawish]

Thanks @thaJeztah I'll update the test

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@6cb75dd). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #36407   +/-   ##
=========================================
  Coverage          ?   34.69%           
=========================================
  Files             ?      614           
  Lines             ?    45490           
  Branches          ?        0           
=========================================
  Hits              ?    15782           
  Misses            ?    27640           
  Partials          ?     2068

[agawish]

I checked the log of powerpc jenkins job and it seems like docker_cli_swarm_test.go is throwing an exception. I don't think this is related to my changes though!

[dnephin]

Instead of repeating the error message in a few places I think this var should be replaced with:

func newErrBindSourceDoesNotExist(path string) {
    return errors.Errorf("bind mount source path does not exist: %s", path)
}

[agawish]

That looks inline with the current implementation of other errors.
Are you happy with this format:

bind mount source path does not exist: %s

or my version of the format is acceptable:

bind mount source path: '%s' does not exist

[dnephin]

I don't have a strong preference. Either is ok with me.

[agawish]

I think I'll go with your format version - I hate quotes too! 😄 - with a slight change to the method name to match other methods naming convention.

[dnephin]

LGTM, Thanks!

[vdemeester]

LGTM 🌵

[thaJeztah]

thanks! changes LGTM, but can you squash your commits? Let me know if you don't know how to, and need help 👍

[agawish]

@thaJeztah wouldn't that mean that I need to create another branch, and/or revert some of these discussions?

[thaJeztah]

@agawish we try to keep changes grouped in logical commits and keep review history ("change name after review", "fix typo" etc) out of the git history.

There's no need to create a new branch; if you do an "interactive" rebase, you can squash commits, then after that's done a force push to update the PR. Here's a section in the contribution guide that explains this; https://docs.docker.com/v17.06/opensource/workflow/work-issue/#pull-and-rebase-frequently

If your "upstream" remote (the remote pointing to the https://github.com/moby/moby repository) is named "upstream", and the remote pointing to your fork is named "origin", the process is:

1. Make sure you're in the correct branch

git checkout 36395-mount-print

2. Make sure your "upstream" and "origin" remotes are up to date

git fetch upstream
git fetch origin

3. Start an interactive rebase on the "upstream" master branch

git rebase -i upstream/master

4. Git opens an editor and shows the commits you added in this PR:

pick 8b9866ef2 Making a dry run test.
pick f038d8bbb Closes: #36395
pick c22740211 Remove the TEST.md file
pick 4bca8a1e2 Fix volume_test.go unit test (2 cases has been changed) to reflect the changes done in #36395
pick cf772e691 Fix docker_api_containers_test.go (1 integration test) to match the new error message
pick d1bb896e5 Refactoring the code to do the following: 1. Add the method `errBindSourceDoesNotExist` inside `validate.go` to be in-line with the rest of error message 2. Utilised the new method$

# Rebase 407e122ac..d1bb896e5 onto 407e122ac (6 commands)
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
# d, drop = remove commit
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

You can see that Git already shows some instructions in that file.

The first ("Making a dry run test") and third ("Remove the TEST.md file") commits are not needed, so you can either "squash" them, or "remove" them.

Removing is simply removing those lines:

pick f038d8bbb Closes: #36395
pick 4bca8a1e2 Fix volume_test.go unit test (2 cases has been changed) to reflect the changes done in #36395
pick cf772e691 Fix docker_api_containers_test.go (1 integration test) to match the new error message
pick d1bb896e5 Refactoring the code to do the following: 1. Add the method `errBindSourceDoesNotExist` inside `validate.go` to be in-line with the rest of error message 2. Utilised the new method$

The remaining commits are all related to this PR, and looking at the changes in this PR, I think it's ok to combine them all in a single commit. So, you'd want to "squash" them together with the first commit;

pick f038d8bbb Closes: #36395
squash 4bca8a1e2 Fix volume_test.go unit test (2 cases has been changed) to reflect the changes done in #36395
squash cf772e691 Fix docker_api_containers_test.go (1 integration test) to match the new error message
squash d1bb896e5 Refactoring the code to do the following: 1. Add the method `errBindSourceDoesNotExist` inside `validate.go` to be in-line with the rest of error message 2. Utilised the new meth$

After you made those changes, save the file.

Git will now open a new editor, that allows you to update the commit-message;

# This is a combination of 4 commits.
# This is the 1st commit message:

Closes: #36395

1. Removed errBindNotExist variable as it is not used anymore.
2. Updated linux_parser.go and windows_parser.go to display the Source directory in the error message.
3. Updated validate_test.go with the new error message

Signed-off-by: Amr Gawish <amr.gawish@gmail.com>

# This is the commit message #2:

Fix volume_test.go unit test (2 cases has been changed) to reflect the changes done in #36395

Signed-off-by: Amr Gawish <amr.gawish@gmail.com>

# This is the commit message #3:

Fix docker_api_containers_test.go (1 integration test) to match the new error message

Signed-off-by: Amr Gawish <amr.gawish@gmail.com>

# This is the commit message #4:

Refactoring the code to do the following:
1. Add the method `errBindSourceDoesNotExist` inside `validate.go` to be in-line with the rest of error message
2. Utilised the new method inside `linux_parser.go`, `windows_parser.go` and `validate_test.go`
3. Change the format from `bind mount source path: '%s' does not exist` to `bind mount source path does not exist: %s`
4. Reflected the format change into the 2 unit tests, namely: `volume_test.go` and `validate_test.go`
5. Reflected the format change into `docker_api_containers_test.go` integration test

I would suggest updating the Closes: ... line to a one-line description that describes the change (which makes it easier to understand what is changed when reading the git history, and without having to head to GitHub to find the related issue), for example, change it to: "print which path failed when a mount fails".

After saving the file, the commits are squashed, and Git prints a message that it completed the rebase;

...
 6 files changed, 11 insertions(+), 9 deletions(-)
Successfully rebased and updated refs/heads/agawish-36395-mount-print.

Now, to update your PR, you need to force push your changes (because you've rebased);

git push -f origin

And you're done 👍

[agawish]

Is everything looks fine now @thaJeztah ?

[thaJeztah]

LGTM, thanks!