Support interpolating environment variables #47
Conversation
|
@joshwget You have one Thanks for the contribution, I'll try to provide some feedback tonight. |
| @@ -0,0 +1,161 @@ | |||
| package project | |||
There was a problem hiding this comment.
From what I've seen there is a convention to name the test file interpolation_test.go. What's the reason for deviating from that convention ?
There was a problem hiding this comment.
I thought it was necessary for internal tests. I'll rename it!
| for k, v := range *config { | ||
| for k2, v2 := range v { | ||
| err := interpolate(k2, k, &v2, func(s string) string { | ||
| values := environmentLookup.Lookup(s, k, nil) |
There was a problem hiding this comment.
Wasn't sure what to do with the third argument here since a ServiceConfig isn't created until after interpolation is performed. This works fine using the OsEnvLookup implementation, which is currently the only one, but it might not be a good idea in general.
There was a problem hiding this comment.
Yeah, that makes sense. I think it's fine to say that the serviceConfig could be nil and the impl of EnvironmentLookup so be programmed so.
|
LGTM ping @aduermael @gdevillele |
| testInterpolatedLine(t, "ABC", "${A}", variables) | ||
|
|
||
| testInterpolatedLine(t, "ABC DE", "$A DE", variables) | ||
| testInterpolatedLine(t, "ABCDE", "${A}DE", variables) |
There was a problem hiding this comment.
I would add a testInterpolatedLine(t, "", "$ADE", variables) case
|
@joshwget looks good 😉. Two things before merging 😉 :
WARN[0000] The IMAGE variable is not set. Substituting a blank string.
INFO[0000] [0/1] [base]: Starting
ERRO[0000] Failed Starting base : 500 Internal Server Error: No command specified
ERRO[0000] Failed to start: base : 500 Internal Server Error: No command specified
FATA[0000] 500 Internal Server Error: No command specifiedUsing # […] long stack >_<
do_build=do_build,
File "/code/compose/service.py", line 317, in ensure_image_exists
self.image()
File "/code/compose/service.py", line 332, in image
return self.client.inspect_image(self.image_name)
File "/code/.tox/py27/lib/python2.7/site-packages/docker/utils/decorators.py", line 18, in wrapped
'image or container param is undefined'
docker.errors.NullResource: image or container param is undefinedThe warning is good enough I think but I feel the Anyway, this looks good 😉. Edit : also, could you squash your commits ? 😉 |
Fixes docker#40 Signed-off-by: Josh Curl <hello@joshcurl.com>
joshwget
force-pushed
the
interpolation
branch
from
9b36c38 to
1b47dd8
Compare
|
@joshwget what do you think of my comments on the tests ? 😸 (I'm okay merging without it, i't just nits 😉) |
|
@vdemeester I think the 500 errors will go away when #34 is fixed, and I'm currently working on a PR for that! I also added the test cases you recommended. |
|
@joshwget ah you're right 😝 I might need some sleep or glasses 😅. LGTM |
Support interpolating environment variables
| return "$", pos, true | ||
| case c == '{': | ||
| return parseVariableWithBraces(line, pos+1, mapping) | ||
| case c >= 'A' && c <= 'Z': |
There was a problem hiding this comment.
This is missing lowercase letters, which are valid in variable names.
There was a problem hiding this comment.
I created #70 to address this.
Fixes #40
Signed-off-by: Josh Curl hello@joshcurl.com