Avoid setting default truthy values from flags that are not set. #20471
Conversation
calavera
force-pushed
the
userland_proxy_config_fix
branch
2 times, most recently
from
720113f
to
39384dc
Compare
calavera
force-pushed
the
userland_proxy_config_fix
branch
3 times, most recently
from
6888281
to
caa7742
Compare
|
LGTM 🐰 |
|
janky failed on authz test only, thats a known issue. |
|
AFAICT, mergo.Merge(dst, src) overwrites any field in dst that has zero or false with the src field's value. Wouldnt this affect fields with 0 int values in the fileConfig that have non-zero defaults in flagConfig? i.e issue affects more than just bool types? |
|
@anusha-ragunathan that is correct. However, we only have one https://github.com/docker/docker/blob/master/daemon/config.go#L117 |
|
Ulimit (part of InstallFlags) is also int, but has null initialization. So we should be okay there as well. I'd like a comment about ints, so that in future if we add such a integer flag, we also add a corresponding patch to handle this case. |
| } | ||
| } | ||
| if len(namedOptions) > 0 { | ||
| // set also default for mergeVal flags that are boolValue at the same time. |
There was a problem hiding this comment.
In the above loop, we are looping through the configSet, looking up their corresponding flags and setting flag values.
In this loop, we are looping through all flags, looking up their config values and again setting the flags.
Can you please explain with an example, why the second set is necessary?
There was a problem hiding this comment.
not all option names match 1-to-1 with flag names, like label and labels. We only perform the second loop if there are in fact any of those options specified in the configuration file.
calavera
force-pushed
the
userland_proxy_config_fix
branch
from
caa7742
to
5a5a11f
Compare
I don't think a code comment can prevent that to happen unfortunately. I'd like to have more tests around those kind of cases, but they don't apply here. |
|
There's already a comment about 'config file with nullable values". Add a line about how this behavior affects integers as well. That's it. |
When the value for a configuration option in the file is `false`, and the default value for a flag is `true`, we should not take the value from the later as final value for the option, because the user explicitly set `false`. This change overrides the default value in the flagSet with the value in the configuration file so we get the correct result when we merge the two configurations together. Signed-off-by: David Calavera <david.calavera@gmail.com>
calavera
force-pushed
the
userland_proxy_config_fix
branch
from
5a5a11f
to
31cb96d
Compare
|
LGTM |
|
LGTM! |
Avoid setting default truthy values from flags that are not set.
|
Job: Docker-PRs-WoW-TP4 FAILED: ---
56 docker-1.11.0-dev.exe.md5
0%
100%
New File 88 docker-1.11.0-dev.exe.sha256
0%
100%
New File 27.0 m docker.exe
0.0%
3.6%
7.3%
11.0%
14.7%
18.4%
22.1%
25.8%
29.5%
33.2%
36.9%
40.6%
44.3%
48.0%
51.7%
55.4%
59.1%
62.8%
66.5%
70.2%
73.9%
77.6%
81.3%
85.0%
88.7%
92.4%
96.1%
99.8%
100%
------------------------------------------------------------------------------
Total Copied Skipped Mismatch FAILED Extras
Dirs : 1 0 1 0 0 0
Files : 4 4 0 0 0 0
Bytes : 54.09 m 54.09 m 0 0 0 0
Times : 0:00:00 0:00:00 0:00:00 0:00:00
Speed : 766509540 Bytes/sec.
Speed : 43860.027 MegaBytes/min.
Ended : Saturday, February 20, 2016 1:44:44 AM
+ ec=0
+ set +x
INFO: Linking the built binary to /d/CI/CI-3bc9f
---
|
When the value for a configuration option in the file is
false,and the default value for a flag is
true, we should nottake the value from the later as final value for the option,
because the user explicitly set
false.This change overrides the default value in the flagSet with
the value in the configuration file so we get the correct
result when we merge the two configurations together.
Fixes #20289.
Signed-off-by: David Calavera david.calavera@gmail.com