Tests, refactor and coverage on package opts by vdemeester · Pull Request #13694 · moby/moby

vdemeester · 2015-06-03T07:34:43Z

This is the time for opts to get covered 🐸 and a little bit refactored.

ValidateEnv and ParseEnvFile uses a regexp to valide that it is a correct environment variable that is passed. The definition used :

Variables set by the user must have a name consisting solely of alphabetics, numerics, and underscores - the first of which must not be numeric.

ValidateLabel is made consistent with the LABEL in Dockerfile as it allows multiple = like LABEL label=value1=value2.

# before
+ go test -test.timeout=30m github.com/docker/docker/opts
PASS
coverage: 33.8% of statements
#after
+ go test -test.timeout=30m github.com/docker/docker/opts
PASS
coverage: 92.8% of statements

There is a few FIXME in this one :

~~ValidatePath(":/test") is valid, although it will fail after I think~~
~~func ValidateEnv(val string) (string, error) always returns nil for error. I guess it's for coherence of returns with the other validate method, so I think I can remove my FIXME comment.~~
~~There is FIXMEs for ListOpts.GetAll() and ListOpts.GetMap asking if we can remove it. They are used in runconfig/parse.go ; GetMap could/should probably have another name but..~~

Signed-off-by: Vincent Demeester vincent@sbr.pm

calavera · 2015-06-04T20:07:29Z

opts/opts.go

why don't you fix it rather than leaving a comment? 😉

Yup, and it shouldn't :)

Well, I was asking if this was ok before doing it 😸. Now that I know it is I will fix it 😉.

Ok that's kinda what I guessed, it has to be there because it is used as a type ValidatorFctType func(val string) (string, error). I think I should just remove the comment for now 😝.

Environment(s) and thus Label(s) (that are validated the same way) are always valid, just rewritten a bit sometimes.

duglin · 2015-06-05T16:07:12Z

opts/envfile_test.go

IMO we're inconsistent. If you added a "=" after each of those you'd get an error like:

docker: poorly formatted environment: variable 'a b c' has white spaces.

Yet w/o the "=" it passes it thru to the container. We should either allow both or error both.
I think we should detect the spaces and error in both cases. Want to fix that as part of this?

@duglin I sure do 😉

There is a certain consistency in the fact that we are able to run docker run -e "something with space" busybox /bin/sh ; although the environment variables is not set in the container. I should probably validate this too (ValidateEnv).

vdemeester · 2015-06-05T22:20:31Z

Ok, so I've updated the envfile end env validation.

For envfile, it will fail if there is spaces even without =, a file like this would fail :
```
some spaces
```
Almost the same for env (and now there is a case it returns an error 😝 ), it's no more possible to do something like : docker run -e "some spaces" ….

For the sake of it, I wasn't sure if it was possible to define a environment variable with spaces, so I tried (with bash and zsh) :

$ export SOME\ SPACES=value
bash: export: `SOME SPACES=value': not a valid identifier
$ export "SOME SPACES"=value
bash: export: `SOME SPACES=value': not a valid identifier

☺️

duglin · 2015-06-05T22:23:15Z

yep - that's what I got in my testing as well. thanks!!

rhatdan · 2015-06-08T12:25:11Z

LABEL call just stole code from ENV. It should follow the same standard of whatever ENV allows, I believe.

vdemeester · 2015-06-11T19:53:02Z

Rebased to master and move a test from #13879 in ip_test.go.

duglin · 2015-06-11T21:31:54Z

opts/envfile.go

minor thing, and up to you, but I think if len(data) > 1 would be better just because it'll be faster than looping over the string looking for =

sounds about right, will do :)

duglin · 2015-07-09T13:23:42Z

opts/opts.go

I think we may need to externalize and then use validMountMode() from daemon/volumes.go since the list of modes is more than just rw and ro. What do you think?

You're right 😸. I'll look at that this evening.

duglin · 2015-07-09T13:33:21Z

Just one question/comment! Looking good!

vdemeester · 2015-07-09T18:30:43Z

runconfig/parse_test.go

This one will not fail anymore, so removing it :).

Meaning, now it is supposed to work with this syntax and not fail like on this test 😉.

vdemeester · 2015-07-09T20:06:26Z

@duglin I think it's done.. I'm not found of the move I made of rwMode and roMode. WDYT ? 😅

duglin · 2015-07-09T21:43:21Z

@vdemeester yea, lets keep that code into volumes.go and just externalize validateMountMode() instead of moving it all into opt.go. To me, volumes (sort of) owns all that logic and the opts stuff is more like a user of it.

vdemeester · 2015-07-10T07:14:35Z

@duglin The problem is there seems to be a cycle loop of import (if I import daemon in the package opts) . I'm gonna move it into github.com/docker/docker/volume.

vdemeester · 2015-07-10T15:07:20Z

@duglin quick update :

Moved ValidateMountMode to github.com/docker/docker/volume.
Fixed few tests (runconfig, etc..), e.g. that would failed because they were considered rwz that would be later rejected as not supported.
Added a bunch of godoc 😉.

- Refactor opts.ValidatePath and add an opts.ValidateDevice ValidePath will now accept : containerPath:mode, hostPath:containerPath:mode and hostPath:containerPath. ValidateDevice will have the same behavior as current. - Refactor opts.ValidateEnv, opts.ParseEnvFile Environment variables will now be validated with the following definition : > Environment variables set by the user must have a name consisting > solely of alphabetics, numerics, and underscores - the first of > which must not be numeric. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester · 2015-07-12T08:42:44Z

@duglin Another update 😅.
I've added a ValidateDevice function that will do the same as ValidatePath but without validating the mount mode (https://github.com/docker/docker/pull/13694/files#diff-def63b7f78492fa1f0dc4e219a650df4R229). It should fix the DockerSuite.TestDevicePermissions integration test.

duglin · 2015-07-14T21:58:30Z

@vdemeester thanks for the hard work!
LGTM

ping @LK4D4 you still ok with this?

calavera · 2015-07-14T22:09:45Z

LGTM

Tests, refactor and coverage on package opts

thaJeztah · 2015-07-14T22:12:54Z

🎉 thanks!!

vdemeester · 2015-07-15T06:35:58Z

🎉 😊
@duglin thanks for all the reviews !

Update documentation according to opts updates (#13694)

thaJeztah · 2015-07-21T12:21:46Z

Added impact labels, because the validation-rules for environment variables changed in this PR (see #14727)

GordonTheTurtle added the status/0-triage label Jun 3, 2015

runcom added status/2-code-review and removed status/0-triage labels Jun 3, 2015

vdemeester force-pushed the opts-test-coverage branch 3 times, most recently from e2990f3 to 1e2ba46 Compare June 3, 2015 08:13

calavera reviewed Jun 4, 2015
View reviewed changes

vdemeester force-pushed the opts-test-coverage branch from 1e2ba46 to 3ee81b8 Compare June 5, 2015 06:42

duglin reviewed Jun 5, 2015
View reviewed changes

vdemeester force-pushed the opts-test-coverage branch 2 times, most recently from a64cde1 to 7d2c339 Compare June 5, 2015 22:03

vdemeester force-pushed the opts-test-coverage branch from 7d2c339 to dcc377d Compare June 7, 2015 19:26

vdemeester mentioned this pull request Jun 11, 2015

Add test coverage to pkg/jsonlog #13876

Merged

vdemeester force-pushed the opts-test-coverage branch from dcc377d to a89c57d Compare June 11, 2015 19:52

duglin reviewed Jun 11, 2015
View reviewed changes

vdemeester force-pushed the opts-test-coverage branch from b3c0d6c to bfca3fe Compare July 9, 2015 12:20

duglin reviewed Jul 9, 2015
View reviewed changes

vdemeester force-pushed the opts-test-coverage branch from bfca3fe to cf9a020 Compare July 9, 2015 16:45

vdemeester reviewed Jul 9, 2015
View reviewed changes

vdemeester force-pushed the opts-test-coverage branch 3 times, most recently from 91fb81f to 37e4bef Compare July 10, 2015 15:00

vdemeester force-pushed the opts-test-coverage branch from 37e4bef to dfc6c04 Compare July 12, 2015 08:34

calavera added a commit that referenced this pull request Jul 14, 2015

Merge pull request #13694 from vdemeester/opts-test-coverage

ecdbf86

Tests, refactor and coverage on package opts

calavera merged commit ecdbf86 into moby:master Jul 14, 2015

vdemeester deleted the opts-test-coverage branch July 15, 2015 05:01

vdemeester mentioned this pull request Jul 18, 2015

Update documentation according to opts updates (#13694) #14727

Merged

thaJeztah added a commit that referenced this pull request Jul 21, 2015

Merge pull request #14727 from vdemeester/opts-docs-updates

1e93941

Update documentation according to opts updates (#13694)

thaJeztah added impact/changelog impact/cli labels Jul 21, 2015

calavera mentioned this pull request Sep 25, 2015

Docker 1.8 - no longer able to pass Spring config settings in env-file #16585

Closed

vdemeester mentioned this pull request Sep 26, 2015

Revert environment regexp in opts #16608

Merged

thaJeztah mentioned this pull request Dec 13, 2017

Raise a warning when using an env file containing variable names with unallowed characters (e.g. a hypen / dash) #35787

Closed

Conversation

vdemeester commented Jun 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Jun 5, 2015

Uh oh!

duglin commented Jun 5, 2015

Uh oh!

rhatdan commented Jun 8, 2015

Uh oh!

vdemeester commented Jun 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Jul 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Jul 9, 2015

Uh oh!

duglin commented Jul 9, 2015

Uh oh!

vdemeester commented Jul 10, 2015

Uh oh!

vdemeester commented Jul 10, 2015

Uh oh!

vdemeester commented Jul 12, 2015

Uh oh!

duglin commented Jul 14, 2015

Uh oh!

calavera commented Jul 14, 2015

Uh oh!

thaJeztah commented Jul 14, 2015

Uh oh!

vdemeester commented Jul 15, 2015

Uh oh!

thaJeztah commented Jul 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants