Avoid setting default truthy values from flags that are not set.

[calavera]

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.

Fixes #20289.

Signed-off-by: David Calavera david.calavera@gmail.com

[vdemeester]

LGTM 🐰

[tiborvass]

janky failed on authz test only, thats a known issue.

[anusha-ragunathan]

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?

[calavera]

@anusha-ragunathan that is correct. However, we only have one Int flag option, Mtu, which default is 0, making it not an issue like boolean flags:

https://github.com/docker/docker/blob/master/daemon/config.go#L117

[anusha-ragunathan]

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.

[anusha-ragunathan]

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?

[calavera]

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]

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.

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.

[anusha-ragunathan]

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.

[tiborvass]

LGTM

[anusha-ragunathan]

LGTM!

[GordonTheTurtle]

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
---