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

added check for empty source in bind mount #824

Merged

Conversation

ethan-haynes
Copy link

@ethan-haynes ethan-haynes commented Jan 21, 2018

Provides a fix for #820
fixes #820

- What I did
Fixed the bug that allowed a user to use a bind mount without a source in the compose file. Without providing a source the loader would add the path of the compose file.
- How I did it
Added error handing and explicit check for empty source in compose file. The loader now throws an error if it detects a bind mount without a source.
- How to verify it

1: make a docker-compose.yml file

version: "3.4"
services:
  nginx:
    image: nginx:alpine
    volumes:
      - type: bind
        target: /app

2: run stack deploy commands

$: docker stack deploy -c docker-compose.yml nginx

The output should be the following error:
invalid mount config for type "bind": field Source must not be empty

@codecov-io
Copy link

codecov-io commented Jan 21, 2018

Codecov Report

Merging #824 into master will decrease coverage by 1.25%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #824      +/-   ##
=========================================
- Coverage   52.95%   51.7%   -1.26%     
=========================================
  Files         244     244              
  Lines       15828   15827       -1     
=========================================
- Hits         8382    8183     -199     
- Misses       6892    7100     +208     
+ Partials      554     544      -10

@ethan-haynes ethan-haynes force-pushed the 820-bind-mount-source-missing-error branch from a4f863b to 28b7d94 Compare January 21, 2018 05:51
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Small nits

for i, volume := range volumes {
if volume.Type != "bind" {
continue
}

if volume.Source == "" {
return errors.New("invalid mount config for type \"bind\": field Source must not be empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

return errors.New(`invalid mount config for type "bind": field Source must not be empty`)

is simpler 😄

- type: bind
target: /app
`)
require.Error(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

require.EqualError(t, err, `invalid mount config for type "bind": field Source must not be empty`)

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

fixed error by removing punctuation

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

removed period

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

backtick string to escape double quotes in error messages

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

simplified test for bind no source error message

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>
@ethan-haynes ethan-haynes force-pushed the 820-bind-mount-source-missing-error branch from 1bafe67 to e76d8c9 Compare January 23, 2018 00:34
Copy link
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

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🌮
cc @dnephin

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.

path to compose file being set as source for bind mount when source is not provided in compose file
7 participants