Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure we marshall version too… #891

Merged
merged 1 commit into from Feb 21, 2018

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

@@ -12,6 +12,15 @@ type versionedConfig struct {
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.

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

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

None yet

6 participants