The Wayback Machine - https://web.archive.org/web/20260314084317/https://github.com/docker/cli/pull/891
Skip to content

Make sure we marshall version too…#891

Merged
dnephin merged 1 commit intodocker:masterfrom
vdemeester:k8s-loader-make-sure-version
Feb 21, 2018
Merged

Make sure we marshall version too…#891
dnephin merged 1 commit intodocker:masterfrom
vdemeester:k8s-loader-make-sure-version

Conversation

@vdemeester
Copy link
Collaborator

… otherwise the k8s controller might fail to parse the file as it will
think it's version 1.

Signed-off-by: Vincent Demeester vincent@sbr.pm

@codecov-io
Copy link

codecov-io commented Feb 20, 2018

Codecov Report

Merging #891 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #891      +/-   ##
==========================================
+ Coverage   53.21%   53.23%   +0.02%     
==========================================
  Files         258      258              
  Lines       16346    16349       +3     
==========================================
+ Hits         8698     8704       +6     
+ Misses       7083     7080       -3     
  Partials      565      565

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

lol, yup LGTM

Version string
}

func (c versionedConfig) MarshalYAML() (interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem is that composetypes.Config is being used as an embeded struct, and it has a MarshalYAML so that is what is called when this versionedConfig struct is marshalled.

How about:

diff --git a/cli/command/stack/kubernetes/loader.go b/cli/command/stack/kubernetes/loader.go
index 14aec58914..66a4071792 100644
--- a/cli/command/stack/kubernetes/loader.go
+++ b/cli/command/stack/kubernetes/loader.go
@@ -8,17 +8,26 @@ import (
 )
 
 type versionedConfig struct {
-	*composetypes.Config `yaml:",inline"`
-	Version              string
+	composetypes.Config
+	Version string
 }
 
+// MarshalYAML makes Config implement yaml.Marshaller
 func (c versionedConfig) MarshalYAML() (interface{}, error) {
-	i, err := c.Config.MarshalYAML()
-	if err != nil {
-		return nil, err
+	m := map[string]interface{}{}
+	services := map[string]composetypes.ServiceConfig{}
+	for _, service := range c.Services {
+		s := service
+		s.Name = ""
+		services[service.Name] = s
 	}
-	i.(map[string]interface{})["version"] = c.Version
-	return i, nil
+	m["services"] = services
+	m["networks"] = c.Networks
+	m["volumes"] = c.Volumes
+	m["secrets"] = c.Secrets
+	m["configs"] = c.Configs
+	m["version"] = c.Version
+	return m, nil
 }
 
 // LoadStack loads a stack from a Compose config, with a given name.
@@ -26,7 +35,7 @@ func LoadStack(name, version string, cfg composetypes.Config) (*apiv1beta1.Stack
 	cfg.Filename = ""
 	res, err := yaml.Marshal(versionedConfig{
 		Version: version,
-		Config:  &cfg,
+		Config:  cfg,
 	})
 	if err != nil {
 		return nil, err
diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go
index 2299871cfb..37ec1b5dd7 100644
--- a/cli/compose/types/types.go
+++ b/cli/compose/types/types.go
@@ -78,23 +78,6 @@ type Config struct {
 	Configs  map[string]ConfigObjConfig
 }
 
-// MarshalYAML makes Config implement yaml.Marshaller
-func (c *Config) MarshalYAML() (interface{}, error) {
-	m := map[string]interface{}{}
-	services := map[string]ServiceConfig{}
-	for _, service := range c.Services {
-		s := service
-		s.Name = ""
-		services[service.Name] = s
-	}
-	m["services"] = services
-	m["networks"] = c.Networks
-	m["volumes"] = c.Volumes
-	m["secrets"] = c.Secrets
-	m["configs"] = c.Configs
-	return m, nil
-}
-
 // ServiceConfig is the configuration of one service
 type ServiceConfig struct {
 	Name string `yaml:",omitempty"`

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could make a type for Config.Services that has the MarshalYAML on it, add a yaml:"-" to Config.Filename, and then we should be able to just use inline here.

@vdemeester vdemeester force-pushed the k8s-loader-make-sure-version branch from 568167e to 4a1df95 Compare February 21, 2018 10:19
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Some nits

m := map[string]interface{}{}
services := map[string]composetypes.ServiceConfig{}
for _, service := range c.Services {
s := service
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you copy service here?

s := service
services[service.Name] = s
}
m["services"] = services
Copy link
Contributor

Choose a reason for hiding this comment

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

return map[string]interface{}{
  "networks": c.Networks,
  "volumes": c.Volumes,
...
}, nil

@vdemeester vdemeester force-pushed the k8s-loader-make-sure-version branch from 4a1df95 to 68267e5 Compare February 21, 2018 14:35
… otherwise the k8s controller might fail to parse the file as it will
think it's version 1.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester force-pushed the k8s-loader-make-sure-version branch from 68267e5 to 9f9f1c8 Compare February 21, 2018 14:37
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

version string
}

func (c versionedConfig) MarshalYAML() (interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll be able to remove these MarshalYAML in the future by making Config.Services a type with a MarshalYAML.

@dnephin dnephin merged commit 939938b into docker:master Feb 21, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.03.0 milestone Feb 21, 2018
@vdemeester vdemeester deleted the k8s-loader-make-sure-version branch February 21, 2018 16:45
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
…rsion

Make sure we marshall version too…
Upstream-commit: 939938b
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants