From ea43c33330ec9a1e9a9b8a85348c1757fdae65c4 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Tue, 7 Feb 2017 09:44:47 +0000 Subject: [PATCH 1/3] compose: fix environment interpolation from the client For an environment variable defined in the yaml without value, the value needs to be propagated from the client, as in Docker Compose. Signed-off-by: Akihiro Suda --- cli/command/stack/deploy_composefile.go | 10 +++ cli/compose/loader/example2.env | 3 + cli/compose/loader/loader.go | 73 ++++++++++++++-------- cli/compose/loader/loader_test.go | 82 +++++++++++++------------ 4 files changed, 104 insertions(+), 64 deletions(-) diff --git a/cli/command/stack/deploy_composefile.go b/cli/command/stack/deploy_composefile.go index 3e62494325..b176b47e0b 100644 --- a/cli/command/stack/deploy_composefile.go +++ b/cli/command/stack/deploy_composefile.go @@ -115,6 +115,16 @@ func getConfigDetails(opts deployOptions) (composetypes.ConfigDetails, error) { } // TODO: support multiple files details.ConfigFiles = []composetypes.ConfigFile{*configFile} + env := os.Environ() + details.Environment = make(map[string]string, len(env)) + for _, s := range env { + // if value is empty, s is like "K=", not "K". + if !strings.Contains(s, "=") { + return details, fmt.Errorf("unexpected environment %q", s) + } + kv := strings.SplitN(s, "=", 2) + details.Environment[kv[0]] = kv[1] + } return details, nil } diff --git a/cli/compose/loader/example2.env b/cli/compose/loader/example2.env index 0920d5ab05..642334e9fd 100644 --- a/cli/compose/loader/example2.env +++ b/cli/compose/loader/example2.env @@ -1 +1,4 @@ BAR=2 + +# overridden in configDetails.Environment +QUX=1 diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index 995047e8c9..7c8bfa0a2a 100644 --- a/cli/compose/loader/loader.go +++ b/cli/compose/loader/loader.go @@ -2,15 +2,16 @@ package loader import ( "fmt" - "os" "path" "reflect" "regexp" "sort" "strings" + "github.com/Sirupsen/logrus" "github.com/docker/docker/cli/compose/interpolation" "github.com/docker/docker/cli/compose/schema" + "github.com/docker/docker/cli/compose/template" "github.com/docker/docker/cli/compose/types" "github.com/docker/docker/opts" runconfigopts "github.com/docker/docker/runconfig/opts" @@ -69,13 +70,17 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) { } cfg := types.Config{} + lookupEnv := func(k string) (string, bool) { + v, ok := configDetails.Environment[k] + return v, ok + } if services, ok := configDict["services"]; ok { - servicesConfig, err := interpolation.Interpolate(services.(types.Dict), "service", os.LookupEnv) + servicesConfig, err := interpolation.Interpolate(services.(types.Dict), "service", lookupEnv) if err != nil { return nil, err } - servicesList, err := LoadServices(servicesConfig, configDetails.WorkingDir) + servicesList, err := LoadServices(servicesConfig, configDetails.WorkingDir, lookupEnv) if err != nil { return nil, err } @@ -84,7 +89,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) { } if networks, ok := configDict["networks"]; ok { - networksConfig, err := interpolation.Interpolate(networks.(types.Dict), "network", os.LookupEnv) + networksConfig, err := interpolation.Interpolate(networks.(types.Dict), "network", lookupEnv) if err != nil { return nil, err } @@ -98,7 +103,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) { } if volumes, ok := configDict["volumes"]; ok { - volumesConfig, err := interpolation.Interpolate(volumes.(types.Dict), "volume", os.LookupEnv) + volumesConfig, err := interpolation.Interpolate(volumes.(types.Dict), "volume", lookupEnv) if err != nil { return nil, err } @@ -112,7 +117,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) { } if secrets, ok := configDict["secrets"]; ok { - secretsConfig, err := interpolation.Interpolate(secrets.(types.Dict), "secret", os.LookupEnv) + secretsConfig, err := interpolation.Interpolate(secrets.(types.Dict), "secret", lookupEnv) if err != nil { return nil, err } @@ -308,11 +313,11 @@ func formatInvalidKeyError(keyPrefix string, key interface{}) error { // LoadServices produces a ServiceConfig map from a compose file Dict // the servicesDict is not validated if directly used. Use Load() to enable validation -func LoadServices(servicesDict types.Dict, workingDir string) ([]types.ServiceConfig, error) { +func LoadServices(servicesDict types.Dict, workingDir string, lookupEnv template.Mapping) ([]types.ServiceConfig, error) { var services []types.ServiceConfig for name, serviceDef := range servicesDict { - serviceConfig, err := LoadService(name, serviceDef.(types.Dict), workingDir) + serviceConfig, err := loadService(name, serviceDef.(types.Dict), workingDir, lookupEnv) if err != nil { return nil, err } @@ -324,22 +329,39 @@ func LoadServices(servicesDict types.Dict, workingDir string) ([]types.ServiceCo // LoadService produces a single ServiceConfig from a compose file Dict // the serviceDict is not validated if directly used. Use Load() to enable validation -func LoadService(name string, serviceDict types.Dict, workingDir string) (*types.ServiceConfig, error) { +func LoadService(name string, serviceDict types.Dict, workingDir string, lookupEnv template.Mapping) (*types.ServiceConfig, error) { serviceConfig := &types.ServiceConfig{} if err := transform(serviceDict, serviceConfig); err != nil { return nil, err } serviceConfig.Name = name - if err := resolveEnvironment(serviceConfig, workingDir); err != nil { + if err := resolveEnvironment(serviceConfig, workingDir, lookupEnv); err != nil { return nil, err } - resolveVolumePaths(serviceConfig.Volumes, workingDir) + resolveVolumePaths(serviceConfig.Volumes, workingDir, lookupEnv) return serviceConfig, nil } -func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string) error { +func updateEnvironment(environment map[string]string, vars map[string]string, lookupEnv template.Mapping) map[string]string { + result := make(map[string]string, len(environment)) + for k, v := range environment { + result[k]=v + } + for k, v := range vars { + interpolatedV, ok := lookupEnv(k) + if ok { + // lookupEnv is prioritized over vars + result[k] = interpolatedV + } else { + result[k] = v + } + } + return result +} + +func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, lookupEnv template.Mapping) error { environment := make(map[string]string) if len(serviceConfig.EnvFile) > 0 { @@ -353,36 +375,35 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string) e } envVars = append(envVars, fileVars...) } - - for k, v := range runconfigopts.ConvertKVStringsToMap(envVars) { - environment[k] = v - } + environment = updateEnvironment(environment, + runconfigopts.ConvertKVStringsToMap(envVars), lookupEnv) } - for k, v := range serviceConfig.Environment { - environment[k] = v - } - - serviceConfig.Environment = environment - + serviceConfig.Environment = updateEnvironment(environment, + serviceConfig.Environment, lookupEnv) return nil } -func resolveVolumePaths(volumes []types.ServiceVolumeConfig, workingDir string) { +func resolveVolumePaths(volumes []types.ServiceVolumeConfig, workingDir string, lookupEnv template.Mapping) { for i, volume := range volumes { if volume.Type != "bind" { continue } - volume.Source = absPath(workingDir, expandUser(volume.Source)) + volume.Source = absPath(workingDir, expandUser(volume.Source, lookupEnv)) volumes[i] = volume } } // TODO: make this more robust -func expandUser(path string) string { +func expandUser(path string, lookupEnv template.Mapping) string { if strings.HasPrefix(path, "~") { - return strings.Replace(path, "~", os.Getenv("HOME"), 1) + home, ok := lookupEnv("HOME") + if !ok { + logrus.Warn("cannot expand '~', because the environment lacks HOME") + return path + } + return strings.Replace(path, "~", home, 1) } return path } diff --git a/cli/compose/loader/loader_test.go b/cli/compose/loader/loader_test.go index b9fb10f227..4f424d6126 100644 --- a/cli/compose/loader/loader_test.go +++ b/cli/compose/loader/loader_test.go @@ -12,7 +12,7 @@ import ( "github.com/stretchr/testify/assert" ) -func buildConfigDetails(source types.Dict) types.ConfigDetails { +func buildConfigDetails(source types.Dict, env map[string]string) types.ConfigDetails { workingDir, err := os.Getwd() if err != nil { panic(err) @@ -23,7 +23,7 @@ func buildConfigDetails(source types.Dict) types.ConfigDetails { ConfigFiles: []types.ConfigFile{ {Filename: "filename.yml", Config: source}, }, - Environment: nil, + Environment: env, } } @@ -154,7 +154,7 @@ func TestParseYAML(t *testing.T) { } func TestLoad(t *testing.T) { - actual, err := Load(buildConfigDetails(sampleDict)) + actual, err := Load(buildConfigDetails(sampleDict, nil)) if !assert.NoError(t, err) { return } @@ -173,7 +173,7 @@ services: secrets: super: external: true -`) +`, nil) if !assert.NoError(t, err) { return } @@ -182,7 +182,7 @@ secrets: } func TestParseAndLoad(t *testing.T) { - actual, err := loadYAML(sampleYAML) + actual, err := loadYAML(sampleYAML, nil) if !assert.NoError(t, err) { return } @@ -192,15 +192,15 @@ func TestParseAndLoad(t *testing.T) { } func TestInvalidTopLevelObjectType(t *testing.T) { - _, err := loadYAML("1") + _, err := loadYAML("1", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "Top-level object must be a mapping") - _, err = loadYAML("\"hello\"") + _, err = loadYAML("\"hello\"", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "Top-level object must be a mapping") - _, err = loadYAML("[\"hello\"]") + _, err = loadYAML("[\"hello\"]", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "Top-level object must be a mapping") } @@ -211,7 +211,7 @@ version: "3" 123: foo: image: busybox -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key at top level: 123") @@ -222,7 +222,7 @@ services: image: busybox 123: image: busybox -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key in services: 123") @@ -236,7 +236,7 @@ networks: ipam: config: - 123: oh dear -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key in networks.default.ipam.config[0]: 123") @@ -247,7 +247,7 @@ services: image: busybox environment: 1: FOO -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key in services.dict-env.environment: 1") } @@ -258,7 +258,7 @@ version: "3" services: foo: image: busybox -`) +`, nil) assert.NoError(t, err) _, err = loadYAML(` @@ -266,7 +266,7 @@ version: "3.0" services: foo: image: busybox -`) +`, nil) assert.NoError(t, err) } @@ -276,7 +276,7 @@ version: "2" services: foo: image: busybox -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "version") @@ -285,7 +285,7 @@ version: "2.0" services: foo: image: busybox -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "version") } @@ -296,7 +296,7 @@ version: 3 services: foo: image: busybox -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "version must be a string") } @@ -305,7 +305,7 @@ func TestV1Unsupported(t *testing.T) { _, err := loadYAML(` foo: image: busybox -`) +`, nil) assert.Error(t, err) } @@ -315,7 +315,7 @@ version: "3" services: - foo: image: busybox -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "services must be a mapping") @@ -323,7 +323,7 @@ services: version: "3" services: foo: busybox -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "services.foo must be a mapping") @@ -332,7 +332,7 @@ version: "3" networks: - default: driver: bridge -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "networks must be a mapping") @@ -340,7 +340,7 @@ networks: version: "3" networks: default: bridge -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "networks.default must be a mapping") @@ -349,7 +349,7 @@ version: "3" volumes: - data: driver: local -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "volumes must be a mapping") @@ -357,7 +357,7 @@ volumes: version: "3" volumes: data: local -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "volumes.data must be a mapping") } @@ -368,7 +368,7 @@ version: "3" services: foo: image: ["busybox", "latest"] -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "services.foo.image must be a string") } @@ -383,6 +383,7 @@ services: FOO: "1" BAR: 2 BAZ: 2.5 + QUX: QUUX: list-env: image: busybox @@ -390,14 +391,16 @@ services: - FOO=1 - BAR=2 - BAZ=2.5 + - QUX - QUUX= -`) +`, map[string]string{"QUX": "qux"}) assert.NoError(t, err) expected := types.MappingWithEquals{ "FOO": "1", "BAR": "2", "BAZ": "2.5", + "QUX": "qux", "QUUX": "", } @@ -416,7 +419,7 @@ services: image: busybox environment: FOO: ["1"] -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "services.dict-env.environment.FOO must be a string, number or null") } @@ -428,12 +431,13 @@ services: dict-env: image: busybox environment: "FOO=1" -`) +`, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "services.dict-env.environment must be a mapping") } func TestEnvironmentInterpolation(t *testing.T) { + home := "/home/foo" config, err := loadYAML(` version: "3" services: @@ -450,12 +454,13 @@ networks: volumes: test: driver: $HOME -`) +`, map[string]string{ + "HOME": home, + "FOO": "foo", + }) assert.NoError(t, err) - home := os.Getenv("HOME") - expectedLabels := types.MappingWithEquals{ "home1": home, "home2": home, @@ -483,7 +488,7 @@ services: `)) assert.NoError(t, err) - configDetails := buildConfigDetails(dict) + configDetails := buildConfigDetails(dict, nil) _, err = Load(configDetails) assert.NoError(t, err) @@ -506,7 +511,7 @@ services: `)) assert.NoError(t, err) - configDetails := buildConfigDetails(dict) + configDetails := buildConfigDetails(dict, nil) _, err = Load(configDetails) assert.NoError(t, err) @@ -529,7 +534,7 @@ services: bar: extends: service: foo -`) +`, nil) assert.Error(t, err) assert.IsType(t, &ForbiddenPropertiesError{}, err) @@ -601,7 +606,8 @@ func TestFullExample(t *testing.T) { bytes, err := ioutil.ReadFile("full-example.yml") assert.NoError(t, err) - config, err := loadYAML(string(bytes)) + homeDir := "/home/foo" + config, err := loadYAML(string(bytes), map[string]string{"HOME": homeDir, "QUX": "2"}) if !assert.NoError(t, err) { return } @@ -609,7 +615,6 @@ func TestFullExample(t *testing.T) { workingDir, err := os.Getwd() assert.NoError(t, err) - homeDir := os.Getenv("HOME") stopGracePeriod := time.Duration(20 * time.Second) expectedServiceConfig := types.ServiceConfig{ @@ -664,6 +669,7 @@ func TestFullExample(t *testing.T) { "FOO": "1", "BAR": "2", "BAZ": "3", + "QUX": "2", }, EnvFile: []string{ "./example1.env", @@ -955,13 +961,13 @@ func TestFullExample(t *testing.T) { assert.Equal(t, expectedVolumeConfig, config.Volumes) } -func loadYAML(yaml string) (*types.Config, error) { +func loadYAML(yaml string, env map[string]string) (*types.Config, error) { dict, err := ParseYAML([]byte(yaml)) if err != nil { return nil, err } - return Load(buildConfigDetails(dict)) + return Load(buildConfigDetails(dict, env)) } func serviceSort(services []types.ServiceConfig) []types.ServiceConfig { From a9c86b63c03fc3557748d7792c11041387cb93d9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 14 Mar 2017 12:39:26 -0400 Subject: [PATCH 2/3] Fix environment resolving. Load from env should only happen if the value is unset. Extract a buildEnvironment function and revert some changes to tests. Signed-off-by: Daniel Nephin --- cli/command/stack/deploy_composefile.go | 18 +++- cli/compose/convert/service.go | 9 +- cli/compose/convert/service_test.go | 10 +- cli/compose/loader/example1.env | 6 +- cli/compose/loader/example2.env | 4 +- cli/compose/loader/full-example.yml | 6 +- cli/compose/loader/loader.go | 79 ++++++++-------- cli/compose/loader/loader_test.go | 118 +++++++++++++----------- cli/compose/types/types.go | 15 +-- 9 files changed, 147 insertions(+), 118 deletions(-) diff --git a/cli/command/stack/deploy_composefile.go b/cli/command/stack/deploy_composefile.go index b176b47e0b..f415f42f8c 100644 --- a/cli/command/stack/deploy_composefile.go +++ b/cli/command/stack/deploy_composefile.go @@ -15,6 +15,7 @@ import ( composetypes "github.com/docker/docker/cli/compose/types" apiclient "github.com/docker/docker/client" dockerclient "github.com/docker/docker/client" + "github.com/pkg/errors" "golang.org/x/net/context" ) @@ -115,17 +116,24 @@ func getConfigDetails(opts deployOptions) (composetypes.ConfigDetails, error) { } // TODO: support multiple files details.ConfigFiles = []composetypes.ConfigFile{*configFile} - env := os.Environ() - details.Environment = make(map[string]string, len(env)) + details.Environment, err = buildEnvironment(os.Environ()) + if err != nil { + return details, err + } + return details, nil +} + +func buildEnvironment(env []string) (map[string]string, error) { + result := make(map[string]string, len(env)) for _, s := range env { // if value is empty, s is like "K=", not "K". if !strings.Contains(s, "=") { - return details, fmt.Errorf("unexpected environment %q", s) + return result, errors.Errorf("unexpected environment %q", s) } kv := strings.SplitN(s, "=", 2) - details.Environment[kv[0]] = kv[1] + result[kv[0]] = kv[1] } - return details, nil + return result, nil } func getConfigFile(filename string) (*composetypes.ConfigFile, error) { diff --git a/cli/compose/convert/service.go b/cli/compose/convert/service.go index ab90d7319a..6b542f7701 100644 --- a/cli/compose/convert/service.go +++ b/cli/compose/convert/service.go @@ -393,11 +393,16 @@ func convertEndpointSpec(endpointMode string, source []composetypes.ServicePortC }, nil } -func convertEnvironment(source map[string]string) []string { +func convertEnvironment(source map[string]*string) []string { var output []string for name, value := range source { - output = append(output, fmt.Sprintf("%s=%s", name, value)) + switch value { + case nil: + output = append(output, name) + default: + output = append(output, fmt.Sprintf("%s=%s", name, *value)) + } } return output diff --git a/cli/compose/convert/service_test.go b/cli/compose/convert/service_test.go index 56f495df3f..352e9a61b5 100644 --- a/cli/compose/convert/service_test.go +++ b/cli/compose/convert/service_test.go @@ -43,10 +43,14 @@ func TestConvertRestartPolicyFromFailure(t *testing.T) { assert.DeepEqual(t, policy, expected) } +func strPtr(val string) *string { + return &val +} + func TestConvertEnvironment(t *testing.T) { - source := map[string]string{ - "foo": "bar", - "key": "value", + source := map[string]*string{ + "foo": strPtr("bar"), + "key": strPtr("value"), } env := convertEnvironment(source) sort.Strings(env) diff --git a/cli/compose/loader/example1.env b/cli/compose/loader/example1.env index 3e7a059613..f19ec0df4e 100644 --- a/cli/compose/loader/example1.env +++ b/cli/compose/loader/example1.env @@ -1,8 +1,8 @@ # passed through -FOO=1 +FOO=foo_from_env_file # overridden in example2.env -BAR=1 +BAR=bar_from_env_file # overridden in full-example.yml -BAZ=1 +BAZ=baz_from_env_file diff --git a/cli/compose/loader/example2.env b/cli/compose/loader/example2.env index 642334e9fd..f47d1e6145 100644 --- a/cli/compose/loader/example2.env +++ b/cli/compose/loader/example2.env @@ -1,4 +1,4 @@ -BAR=2 +BAR=bar_from_env_file_2 # overridden in configDetails.Environment -QUX=1 +QUX=quz_from_env_file_2 diff --git a/cli/compose/loader/full-example.yml b/cli/compose/loader/full-example.yml index fb5686a380..e8f3716013 100644 --- a/cli/compose/loader/full-example.yml +++ b/cli/compose/loader/full-example.yml @@ -77,10 +77,8 @@ services: # Mapping values can be strings, numbers or null # Booleans are not allowed - must be quoted environment: - RACK_ENV: development - SHOW: 'true' - SESSION_SECRET: - BAZ: 3 + BAZ: baz_from_service_def + QUX: # environment: # - RACK_ENV=development # - SHOW=true diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index 7c8bfa0a2a..3edcd81668 100644 --- a/cli/compose/loader/loader.go +++ b/cli/compose/loader/loader.go @@ -253,9 +253,11 @@ func transformHook( case reflect.TypeOf(map[string]*types.ServiceNetworkConfig{}): return transformServiceNetworkMap(data) case reflect.TypeOf(types.MappingWithEquals{}): - return transformMappingOrList(data, "="), nil + return transformMappingOrList(data, "=", true), nil + case reflect.TypeOf(types.Labels{}): + return transformMappingOrList(data, "=", false), nil case reflect.TypeOf(types.MappingWithColon{}): - return transformMappingOrList(data, ":"), nil + return transformMappingOrList(data, ":", false), nil case reflect.TypeOf(types.ServiceVolumeConfig{}): return transformServiceVolumeConfig(data) } @@ -317,7 +319,7 @@ func LoadServices(servicesDict types.Dict, workingDir string, lookupEnv template var services []types.ServiceConfig for name, serviceDef := range servicesDict { - serviceConfig, err := loadService(name, serviceDef.(types.Dict), workingDir, lookupEnv) + serviceConfig, err := LoadService(name, serviceDef.(types.Dict), workingDir, lookupEnv) if err != nil { return nil, err } @@ -344,25 +346,20 @@ func LoadService(name string, serviceDict types.Dict, workingDir string, lookupE return serviceConfig, nil } -func updateEnvironment(environment map[string]string, vars map[string]string, lookupEnv template.Mapping) map[string]string { - result := make(map[string]string, len(environment)) - for k, v := range environment { - result[k]=v - } +func updateEnvironment(environment map[string]*string, vars map[string]*string, lookupEnv template.Mapping) { for k, v := range vars { interpolatedV, ok := lookupEnv(k) - if ok { + if (v == nil || *v == "") && ok { // lookupEnv is prioritized over vars - result[k] = interpolatedV + environment[k] = &interpolatedV } else { - result[k] = v + environment[k] = v } } - return result } func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, lookupEnv template.Mapping) error { - environment := make(map[string]string) + environment := make(map[string]*string) if len(serviceConfig.EnvFile) > 0 { var envVars []string @@ -375,12 +372,12 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, l } envVars = append(envVars, fileVars...) } - environment = updateEnvironment(environment, - runconfigopts.ConvertKVStringsToMap(envVars), lookupEnv) + updateEnvironment(environment, + runconfigopts.ConvertKVStringsToMapWithNil(envVars), lookupEnv) } - serviceConfig.Environment = updateEnvironment(environment, - serviceConfig.Environment, lookupEnv) + updateEnvironment(environment, serviceConfig.Environment, lookupEnv) + serviceConfig.Environment = environment return nil } @@ -497,9 +494,9 @@ func absPath(workingDir string, filepath string) string { func transformMapStringString(data interface{}) (interface{}, error) { switch value := data.(type) { case map[string]interface{}: - return toMapStringString(value), nil + return toMapStringString(value, false), nil case types.Dict: - return toMapStringString(value), nil + return toMapStringString(value, false), nil case map[string]string: return value, nil default: @@ -613,23 +610,27 @@ func transformStringList(data interface{}) (interface{}, error) { } } -func transformMappingOrList(mappingOrList interface{}, sep string) map[string]string { - if mapping, ok := mappingOrList.(types.Dict); ok { - return toMapStringString(mapping) - } - if list, ok := mappingOrList.([]interface{}); ok { - result := make(map[string]string) - for _, value := range list { +func transformMappingOrList(mappingOrList interface{}, sep string, allowNil bool) interface{} { + switch value := mappingOrList.(type) { + case types.Dict: + return toMapStringString(value, allowNil) + case ([]interface{}): + result := make(map[string]interface{}) + for _, value := range value { parts := strings.SplitN(value.(string), sep, 2) - if len(parts) == 1 { - result[parts[0]] = "" - } else { - result[parts[0]] = parts[1] + key := parts[0] + switch { + case len(parts) == 1 && allowNil: + result[key] = nil + case len(parts) == 1 && !allowNil: + result[key] = "" + default: + result[key] = parts[1] } } return result } - panic(fmt.Errorf("expected a map or a slice, got: %#v", mappingOrList)) + panic(fmt.Errorf("expected a map or a list, got %T: %#v", mappingOrList, mappingOrList)) } func transformShellCommand(value interface{}) (interface{}, error) { @@ -693,17 +694,21 @@ func toServicePortConfigs(value string) ([]interface{}, error) { return portConfigs, nil } -func toMapStringString(value map[string]interface{}) map[string]string { - output := make(map[string]string) +func toMapStringString(value map[string]interface{}, allowNil bool) map[string]interface{} { + output := make(map[string]interface{}) for key, value := range value { - output[key] = toString(value) + output[key] = toString(value, allowNil) } return output } -func toString(value interface{}) string { - if value == nil { +func toString(value interface{}, allowNil bool) interface{} { + switch { + case value != nil: + return fmt.Sprint(value) + case allowNil: + return nil + default: return "" } - return fmt.Sprint(value) } diff --git a/cli/compose/loader/loader_test.go b/cli/compose/loader/loader_test.go index 4f424d6126..661e2c615c 100644 --- a/cli/compose/loader/loader_test.go +++ b/cli/compose/loader/loader_test.go @@ -27,6 +27,19 @@ func buildConfigDetails(source types.Dict, env map[string]string) types.ConfigDe } } +func loadYAML(yaml string) (*types.Config, error) { + return loadYAMLWithEnv(yaml, nil) +} + +func loadYAMLWithEnv(yaml string, env map[string]string) (*types.Config, error) { + dict, err := ParseYAML([]byte(yaml)) + if err != nil { + return nil, err + } + + return Load(buildConfigDetails(dict, env)) +} + var sampleYAML = ` version: "3" services: @@ -98,12 +111,16 @@ var sampleDict = types.Dict{ }, } +func strPtr(val string) *string { + return &val +} + var sampleConfig = types.Config{ Services: []types.ServiceConfig{ { Name: "foo", Image: "busybox", - Environment: map[string]string{}, + Environment: map[string]*string{}, Networks: map[string]*types.ServiceNetworkConfig{ "with_me": nil, }, @@ -111,7 +128,7 @@ var sampleConfig = types.Config{ { Name: "bar", Image: "busybox", - Environment: map[string]string{"FOO": "1"}, + Environment: map[string]*string{"FOO": strPtr("1")}, Networks: map[string]*types.ServiceNetworkConfig{ "with_ipam": nil, }, @@ -173,7 +190,7 @@ services: secrets: super: external: true -`, nil) +`) if !assert.NoError(t, err) { return } @@ -182,7 +199,7 @@ secrets: } func TestParseAndLoad(t *testing.T) { - actual, err := loadYAML(sampleYAML, nil) + actual, err := loadYAML(sampleYAML) if !assert.NoError(t, err) { return } @@ -192,15 +209,15 @@ func TestParseAndLoad(t *testing.T) { } func TestInvalidTopLevelObjectType(t *testing.T) { - _, err := loadYAML("1", nil) + _, err := loadYAML("1") assert.Error(t, err) assert.Contains(t, err.Error(), "Top-level object must be a mapping") - _, err = loadYAML("\"hello\"", nil) + _, err = loadYAML("\"hello\"") assert.Error(t, err) assert.Contains(t, err.Error(), "Top-level object must be a mapping") - _, err = loadYAML("[\"hello\"]", nil) + _, err = loadYAML("[\"hello\"]") assert.Error(t, err) assert.Contains(t, err.Error(), "Top-level object must be a mapping") } @@ -211,7 +228,7 @@ version: "3" 123: foo: image: busybox -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key at top level: 123") @@ -222,7 +239,7 @@ services: image: busybox 123: image: busybox -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key in services: 123") @@ -236,7 +253,7 @@ networks: ipam: config: - 123: oh dear -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key in networks.default.ipam.config[0]: 123") @@ -247,7 +264,7 @@ services: image: busybox environment: 1: FOO -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "Non-string key in services.dict-env.environment: 1") } @@ -258,7 +275,7 @@ version: "3" services: foo: image: busybox -`, nil) +`) assert.NoError(t, err) _, err = loadYAML(` @@ -266,7 +283,7 @@ version: "3.0" services: foo: image: busybox -`, nil) +`) assert.NoError(t, err) } @@ -276,7 +293,7 @@ version: "2" services: foo: image: busybox -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "version") @@ -285,7 +302,7 @@ version: "2.0" services: foo: image: busybox -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "version") } @@ -296,7 +313,7 @@ version: 3 services: foo: image: busybox -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "version must be a string") } @@ -305,7 +322,7 @@ func TestV1Unsupported(t *testing.T) { _, err := loadYAML(` foo: image: busybox -`, nil) +`) assert.Error(t, err) } @@ -315,7 +332,7 @@ version: "3" services: - foo: image: busybox -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "services must be a mapping") @@ -323,7 +340,7 @@ services: version: "3" services: foo: busybox -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "services.foo must be a mapping") @@ -332,7 +349,7 @@ version: "3" networks: - default: driver: bridge -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "networks must be a mapping") @@ -340,7 +357,7 @@ networks: version: "3" networks: default: bridge -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "networks.default must be a mapping") @@ -349,7 +366,7 @@ version: "3" volumes: - data: driver: local -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "volumes must be a mapping") @@ -357,7 +374,7 @@ volumes: version: "3" volumes: data: local -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "volumes.data must be a mapping") } @@ -368,13 +385,13 @@ version: "3" services: foo: image: ["busybox", "latest"] -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "services.foo.image must be a string") } -func TestValidEnvironment(t *testing.T) { - config, err := loadYAML(` +func TestLoadWithEnvironment(t *testing.T) { + config, err := loadYAMLWithEnv(` version: "3" services: dict-env: @@ -391,17 +408,17 @@ services: - FOO=1 - BAR=2 - BAZ=2.5 - - QUX - - QUUX= + - QUX= + - QUUX `, map[string]string{"QUX": "qux"}) assert.NoError(t, err) expected := types.MappingWithEquals{ - "FOO": "1", - "BAR": "2", - "BAZ": "2.5", - "QUX": "qux", - "QUUX": "", + "FOO": strPtr("1"), + "BAR": strPtr("2"), + "BAZ": strPtr("2.5"), + "QUX": strPtr("qux"), + "QUUX": nil, } assert.Equal(t, 2, len(config.Services)) @@ -419,7 +436,7 @@ services: image: busybox environment: FOO: ["1"] -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "services.dict-env.environment.FOO must be a string, number or null") } @@ -431,14 +448,14 @@ services: dict-env: image: busybox environment: "FOO=1" -`, nil) +`) assert.Error(t, err) assert.Contains(t, err.Error(), "services.dict-env.environment must be a mapping") } func TestEnvironmentInterpolation(t *testing.T) { home := "/home/foo" - config, err := loadYAML(` + config, err := loadYAMLWithEnv(` version: "3" services: test: @@ -461,7 +478,7 @@ volumes: assert.NoError(t, err) - expectedLabels := types.MappingWithEquals{ + expectedLabels := types.Labels{ "home1": home, "home2": home, "nonexistent": "", @@ -534,7 +551,7 @@ services: bar: extends: service: foo -`, nil) +`) assert.Error(t, err) assert.IsType(t, &ForbiddenPropertiesError{}, err) @@ -607,7 +624,8 @@ func TestFullExample(t *testing.T) { assert.NoError(t, err) homeDir := "/home/foo" - config, err := loadYAML(string(bytes), map[string]string{"HOME": homeDir, "QUX": "2"}) + env := map[string]string{"HOME": homeDir, "QUX": "qux_from_environment"} + config, err := loadYAMLWithEnv(string(bytes), env) if !assert.NoError(t, err) { return } @@ -662,14 +680,11 @@ func TestFullExample(t *testing.T) { DNSSearch: []string{"dc1.example.com", "dc2.example.com"}, DomainName: "foo.com", Entrypoint: []string{"/code/entrypoint.sh", "-p", "3000"}, - Environment: map[string]string{ - "RACK_ENV": "development", - "SHOW": "true", - "SESSION_SECRET": "", - "FOO": "1", - "BAR": "2", - "BAZ": "3", - "QUX": "2", + Environment: map[string]*string{ + "FOO": strPtr("foo_from_env_file"), + "BAR": strPtr("bar_from_env_file_2"), + "BAZ": strPtr("baz_from_service_def"), + "QUX": strPtr("qux_from_environment"), }, EnvFile: []string{ "./example1.env", @@ -961,15 +976,6 @@ func TestFullExample(t *testing.T) { assert.Equal(t, expectedVolumeConfig, config.Volumes) } -func loadYAML(yaml string, env map[string]string) (*types.Config, error) { - dict, err := ParseYAML([]byte(yaml)) - if err != nil { - return nil, err - } - - return Load(buildConfigDetails(dict, env)) -} - func serviceSort(services []types.ServiceConfig) []types.ServiceConfig { sort.Sort(servicesByName(services)) return services diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go index e91b5a7ac8..bb12f8497b 100644 --- a/cli/compose/types/types.go +++ b/cli/compose/types/types.go @@ -99,7 +99,7 @@ type ServiceConfig struct { HealthCheck *HealthCheckConfig Image string Ipc string - Labels MappingWithEquals + Labels Labels Links []string Logging *LoggingConfig MacAddress string `mapstructure:"mac_address"` @@ -135,7 +135,10 @@ type StringOrNumberList []string // MappingWithEquals is a mapping type that can be converted from a list of // key=value strings -type MappingWithEquals map[string]string +type MappingWithEquals map[string]*string + +// Labels is a mapping type for labels +type Labels map[string]string // MappingWithColon is a mapping type that can be converted from a list of // 'key: value' strings @@ -151,7 +154,7 @@ type LoggingConfig struct { type DeployConfig struct { Mode string Replicas *uint64 - Labels MappingWithEquals + Labels Labels UpdateConfig *UpdateConfig `mapstructure:"update_config"` Resources Resources RestartPolicy *RestartPolicy `mapstructure:"restart_policy"` @@ -268,7 +271,7 @@ type NetworkConfig struct { External External Internal bool Attachable bool - Labels MappingWithEquals + Labels Labels } // IPAMConfig for a network @@ -287,7 +290,7 @@ type VolumeConfig struct { Driver string DriverOpts map[string]string `mapstructure:"driver_opts"` External External - Labels MappingWithEquals + Labels Labels } // External identifies a Volume or Network as a reference to a resource that is @@ -301,5 +304,5 @@ type External struct { type SecretConfig struct { File string External External - Labels MappingWithEquals + Labels Labels } From 2cdc9c42eb138bcaffb1cbd4172bd18b69446394 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Fri, 17 Mar 2017 06:21:55 +0000 Subject: [PATCH 3/3] compose: update the comment about MappingWithEquals Signed-off-by: Akihiro Suda --- cli/compose/types/types.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go index bb12f8497b..3e6651fd32 100644 --- a/cli/compose/types/types.go +++ b/cli/compose/types/types.go @@ -134,7 +134,9 @@ type StringList []string type StringOrNumberList []string // MappingWithEquals is a mapping type that can be converted from a list of -// key=value strings +// key[=value] strings. +// For the key with an empty value (`key=`), the mapped value is set to a pointer to `""`. +// For the key without value (`key`), the mapped value is set to nil. type MappingWithEquals map[string]*string // Labels is a mapping type for labels