Fix #3281: Unexpected result when using build args with default values #3449
Conversation
| @@ -2121,7 +2121,7 @@ def test_resolve_environment_from_env_file_with_empty_values(self): | |||
| 'FILE_DEF': u'bär', | |||
| 'FILE_DEF_EMPTY': '', | |||
| 'ENV_DEF': 'E3', | |||
| 'NO_DEF': None | |||
| 'NO_DEF': '' | |||
There was a problem hiding this comment.
This change is wrong, the None is the correct value here, so I think the code change isn't correct to fix the issue.
|
@Andrey9kin Are you up for making the changes @dnephin suggested? We'd like to get this in for 1.8.0! |
|
@aanand last few weeks was quite intense so I didn't have time to submit a fix. |
Fix the issue when build arg is set to None instead of empty string. Usecase: cat docker-compose.yml .... args: - http_proxy - https_proxy - no_proxy If http_proxy, https_proxy, no_proxy environment variables are not defined then http_proxy, https_proxy, no_proxy build args will be set to string None which breaks all downloads With this change build args will not passed to docker engine if they are equal to string None Signed-off-by: Andrey Devyatkin <[email protected]>
| # Moreover it will be sent to the docker engine as None and then | ||
| # interpreted as string None which in many cases will fail the build | ||
| # That is why we filter out all pairs with value equal to None | ||
| buildargs = {k: v for k, v in build_opts.get('args', {}).items() if v != 'None'} |
There was a problem hiding this comment.
I think this should be comparing against the literal value None, not the string 'None'?
|
OK, I've had a look. I think the right place to fix this is at the config parsing stage, rather than in Here's a fix: diff --git a/compose/utils.py b/compose/utils.py
index 494beea..6718d5c 100644
--- a/compose/utils.py
+++ b/compose/utils.py
@@ -95,4 +95,6 @@ def microseconds_from_time_nano(time_nano):
def build_string_dict(source_dict):
- return dict((k, str(v)) for k, v in source_dict.items())
+ def to_str(value):
+ return '' if value is None else value
+ return dict((k, to_str(v)) for k, v in source_dict.items())I think the correct thing to do is pass unset args through as the empty string, not omit them entirely. This is consistent with how both |
|
@aanand sure. Would it be Ok if I separate those two fixes into two pull requests so we have good logical division between issues? |
|
I think the fix you've currently got to |
Fix the issue when build arg is set to None instead of empty string. Usecase: cat docker-compose.yml .... args: - http_proxy - https_proxy - no_proxy If http_proxy, https_proxy, no_proxy environment variables are not defined then http_proxy, https_proxy, no_proxy build args will be set to string None which breaks all downloads With this change undefined build args will be set to empty string instead of string None Signed-off-by: Andrey Devyatkin <[email protected]>
|
@aanand thanks for your guidance. I also felt that it should be a way to implement it better. Please take a look on updated version |
|
LGTM |
| @@ -95,4 +95,4 @@ def microseconds_from_time_nano(time_nano): | |||
|
|
|||
|
|
|||
| def build_string_dict(source_dict): | |||
| return dict((k, str(v)) for k, v in source_dict.items()) | |||
| return dict((k, str(v if v else '')) for k, v in source_dict.items()) | |||
There was a problem hiding this comment.
Only problem with this is it'll result in the empty string for the integer 0, which probably isn't what the user expects. v if v is not None else '' is probably better. (Would be good to add an arg with a value of 0 to the test, too.)
Fix the issue when build arg is set to None instead of empty string. Usecase: cat docker-compose.yml .... args: - http_proxy - https_proxy - no_proxy If http_proxy, https_proxy, no_proxy environment variables are not defined then http_proxy, https_proxy, no_proxy build args will be set to string None which breaks all downloads With this change undefined build args will be set to empty string instead of string None Signed-off-by: Andrey Devyatkin <[email protected]>
|
@aanand missed that. Thank you for your comment. Updated accordingly |
|
LGTM 👍 |
|
LGTM |
Fix the issue when build arg is set to None instead of empty string.
Usecase:
cat docker-compose.yml
....
args:
- http_proxy
- https_proxy
- no_proxy
If http_proxy, https_proxy, no_proxy environment variables are not defined
then http_proxy, https_proxy, no_proxy build args will be set to None which breaks all downloads
With this change build args will be set to empty string if corresponding env
variable is not defined
Signed-off-by: Andrey Devyatkin [email protected]