Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions cli/command/stack/config.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package stack

import (
"bytes"
"fmt"

"github.com/docker/cli/cli"
Expand All @@ -11,7 +12,7 @@ import (
composeLoader "github.com/docker/cli/cli/compose/loader"
composetypes "github.com/docker/cli/cli/compose/types"
"github.com/spf13/cobra"
yaml "gopkg.in/yaml.v2"
"gopkg.in/yaml.v3"
)

func newConfigCommand(dockerCli command.Cli) *cobra.Command {
Expand Down Expand Up @@ -54,9 +55,12 @@ func outputConfig(configFiles composetypes.ConfigDetails, skipInterpolation bool
return "", err
}

d, err := yaml.Marshal(&config)
var buf bytes.Buffer
enc := yaml.NewEncoder(&buf)
enc.SetIndent(2)
err = enc.Encode(&config)
if err != nil {
return "", err
}
return string(d), nil
return buf.String(), nil
}
8 changes: 4 additions & 4 deletions cli/command/stack/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ services:
services:
foo:
command:
- cat
- file2.txt
- cat
- file2.txt
image: busybox:1.0
`,
},
Expand All @@ -69,8 +69,8 @@ services:
services:
foo:
command:
- cat
- file2.txt
- cat
- file2.txt
image: busybox:${VERSION}
`,
},
Expand Down
28 changes: 7 additions & 21 deletions cli/compose/loader/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
"github.com/google/shlex"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
yaml "gopkg.in/yaml.v2"
"gopkg.in/yaml.v3"
)

// Options supported by Load
Expand Down Expand Up @@ -53,11 +53,11 @@ func ParseYAML(source []byte) (map[string]any, error) {
if err := yaml.Unmarshal(source, &cfg); err != nil {
return nil, err
}
cfgMap, ok := cfg.(map[any]any)
_, ok := cfg.(map[string]any)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems strange that many tests in the previous PR were failing with top-level object must be a mapping because the tests haven't changed and the compose file used at the time seem ok to me. Even the versions of the module are the same

So this was the reason; YAML v2 returned a map[any]any whereas v3 always returns a map[string]any.

Honestly, I half-expected map[string]any to ALSO be a map[any]any (because string is also an any ? But that's not how Go asserts these.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So actually, to be more correct; it returns a map[string]interface{} but Go is happy considering interface{} == any

if !ok {
return nil, errors.Errorf("top-level object must be a mapping")
}
converted, err := convertToStringKeysRecursive(cfgMap, "")
converted, err := convertToStringKeysRecursive(cfg, "")
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -349,24 +349,20 @@ func createTransformHook(additionalTransformers ...Transformer) mapstructure.Dec

// keys needs to be converted to strings for jsonschema
func convertToStringKeysRecursive(value any, keyPrefix string) (any, error) {
if mapping, ok := value.(map[any]any); ok {
if mapping, ok := value.(map[string]any); ok {
dict := make(map[string]any)
for key, entry := range mapping {
str, ok := key.(string)
if !ok {
return nil, formatInvalidKeyError(keyPrefix, key)
}
Comment on lines 351 to -358
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But while it makes sense too use map[string]any (i.e., YAML keys would be a string, always, even it it looks like an int or a weird string like {object}:, that also means that we can't produce custom errors for those ("you used an integer as key, did you mean something else?").

var newKeyPrefix string
if keyPrefix == "" {
newKeyPrefix = str
newKeyPrefix = key
} else {
newKeyPrefix = fmt.Sprintf("%s.%s", keyPrefix, str)
newKeyPrefix = fmt.Sprintf("%s.%s", keyPrefix, key)
}
convertedEntry, err := convertToStringKeysRecursive(entry, newKeyPrefix)
if err != nil {
return nil, err
}
dict[str] = convertedEntry
dict[key] = convertedEntry
}
return dict, nil
}
Expand All @@ -385,16 +381,6 @@ func convertToStringKeysRecursive(value any, keyPrefix string) (any, error) {
return value, nil
}

func formatInvalidKeyError(keyPrefix string, key any) error {
var location string
if keyPrefix == "" {
location = "at top level"
} else {
location = "in " + keyPrefix
}
return errors.Errorf("non-string key %s: %#v", location, key)
}
Comment on lines -388 to -396
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these errors could no longer work, unless we would reverse the direction ("string" -> "but is it an integer?")


// 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 map[string]any, workingDir string, lookupEnv template.Mapping) ([]types.ServiceConfig, error) {
Expand Down
2 changes: 2 additions & 0 deletions cli/compose/loader/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,8 @@ func TestInvalidTopLevelObjectType(t *testing.T) {
}

func TestNonStringKeys(t *testing.T) {
// FIXME(thaJeztah): opkg.in/yaml.v3, which always unmarshals to a map[string]any, so we cannot produce a customized error for invalid types.
t.Skip("not supported by gopkg.in/yaml.v3, which always unmarshals to a map[string]any")
_, err := loadYAML(`
Comment on lines 334 to 337
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I kept the test for our custom errors (this test runs with badly formed YAML files to test the custom errors).

But mostly as a "reminder" in case someone would like to bite their teeth into implementing something like that.

(but in all honesty, we can probably just remove the test and call it a day; just write proper YAML)

version: "3"
123:
Expand Down
Loading