Skip to content

Fix error handling for bind mount spec parser.#39251

Merged
thaJeztah merged 1 commit intomoby:masterfrom
cpuguy83:these_pretzels_are_making_me_thirsty
May 23, 2019
Merged

Fix error handling for bind mount spec parser.#39251
thaJeztah merged 1 commit intomoby:masterfrom
cpuguy83:these_pretzels_are_making_me_thirsty

Conversation

@cpuguy83
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 commented May 21, 2019

Errors were being ignored and always telling the user that the path
doesn't exist even if it was some other problem, such as a permission
error.

Closes docker/for-linux#674

@cpuguy83 cpuguy83 requested review from thaJeztah and tiborvass May 21, 2019 22:37
@cpuguy83 cpuguy83 mentioned this pull request May 21, 2019
Closed
3 tasks
@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented May 21, 2019

Adding cherry-pick because this is a quick fix for a really confusing situation.

@cpuguy83 cpuguy83 force-pushed the these_pretzels_are_making_me_thirsty branch from 69c6733 to b698469 Compare May 21, 2019 22:59
thaJeztah
thaJeztah previously approved these changes May 22, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented May 22, 2019

arf; looks like it's failing on Windows;

23:09:37 --- FAIL: TestParseMountSpecBindWithFileinfoError (0.00s)
23:09:37     parser_test.go:529: assertion failed: invalid mount path: '/apples' (e.err *errors.errorString) != some error (testErr *errors.errorString)
23:09:37 FAIL

@thaJeztah thaJeztah dismissed their stale review May 22, 2019 12:21

see comment

@cpuguy83 cpuguy83 force-pushed the these_pretzels_are_making_me_thirsty branch from b698469 to 50d8a41 Compare May 22, 2019 17:30
@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented May 22, 2019

:( Github notifications are having issues, CI is not being triggered.

@codecov
Copy link
Copy Markdown

codecov bot commented May 22, 2019 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##             master   #39251   +/-   ##
=========================================
  Coverage          ?   37.04%           
=========================================
  Files             ?      612           
  Lines             ?    45489           
  Branches          ?        0           
=========================================
  Hits              ?    16850           
  Misses            ?    26349           
  Partials          ?     2290

@cpuguy83
Fix error handling for bind mount spec parser.
ebcef28 
Errors were being ignored and always telling the user that the path
doesn't exist even if it was some other problem, such as a permission
error.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 force-pushed the these_pretzels_are_making_me_thirsty branch from 50d8a41 to ebcef28 Compare May 22, 2019 18:54
@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented May 22, 2019

Well, PR is fixed for Windows... Github is still being a bit funny.

thaJeztah
thaJeztah approved these changes May 22, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

janky failure is docker-py, and doesn't look related

This was referenced May 22, 2019
Merged
Merged
kolyshkin
kolyshkin approved these changes May 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 8a208a1 into moby:master May 23, 2019
@cpuguy83 cpuguy83 deleted the these_pretzels_are_making_me_thirsty branch May 23, 2019 18:47
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@kolyshkin kolyshkin kolyshkin approved these changes

@tiborvass tiborvass Awaiting requested review from tiborvass

Assignees

No one assigned

Labels

process/cherry-picked status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

"Path does not exist" when path definitely exists

4 participants

@cpuguy83 @thaJeztah @kolyshkin @GordonTheTurtle