[compose] Share the compose loading code between swarm and k8s stack deploy by vdemeester · Pull Request #845 · docker/cli

vdemeester · 2018-01-29T21:23:00Z

To ensure we are loading the composefile the same wether we are pointing
to swarm or kubernetes, we need to share the loading code between both.

This means though that docker stack deploy pointing to k8s doesn't handle .env anymore (same beahviour as docker stack deploy on swarm).

struct mashall to a valid composefile
testing this on a real kubernetes cluster with the compose kubernetes on.

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

codecov-io · 2018-01-29T21:42:40Z

Codecov Report

Merging #845 into master will increase coverage by 1.84%.
The diff coverage is 43.52%.

@@            Coverage Diff             @@
##           master     #845      +/-   ##
==========================================
+ Coverage   53.19%   55.03%   +1.84%     
==========================================
  Files         246      234      -12     
  Lines       16021    15449     -572     
==========================================
- Hits         8522     8502      -20     
+ Misses       6929     6382     -547     
+ Partials      570      565       -5

vdemeester · 2018-01-29T21:44:23Z

+	if err != nil {
+		return details, err
+	}
+	// TODO: support multiple files


This will be fixed by #569 👼

silvin-lubecki · 2018-01-29T22:21:58Z

-		return nil, nil, errors.Wrapf(err, "cannot load compose file")
-	}
-
+	fmt.Println(string(res))


yeah it was for debugging purpose, I'll remove once not wip anymore

Yeah right... 😏

silvin-lubecki · 2018-01-30T10:55:02Z

-	"github.com/stretchr/testify/require"
-)
-
-func TestPlaceholders(t *testing.T) {


Was this test really useless, as it just disappeared?

Yes 😏 interpolation is already tested in the cli/compose/... packages 😉

silvin-lubecki · 2018-01-30T11:13:36Z

 					return err
 				}
-				return kubernetes.RunDeploy(kli, opts)
+				return kubernetes.RunDeploy(kli, opts, loadComposefile)


I don't like this functor. I think the only reason is to avoid circular dependency between stack package and swarm/kubernetes packages. A more cleaner way could be moving the loader in its own package, like cli/command/stack/loader.

vdemeester · 2018-02-05T07:01:35Z

So… yaml marshaling the types.Config struct from the compose package produce an invalid composefile 🤔 We may need to fix that 😅

dnephin · 2018-02-05T16:17:23Z

Ya, I guess it's not necessarily designed to be round tripped. How is it invalid?

dnephin

LGTM

vdemeester · 2018-02-05T18:06:52Z

Services are marshalled as a list, but they are unmarshalled as a mapping.

This PR is not mergeable until we can fix that 😅

vdemeester · 2018-02-08T16:44:23Z

Updated: it should now generate a valid yaml composefile — also tried it out on docker-for-mac kubernetes and it works !

Next step should be to upgrade the current test with everything, probably reuse full-example yaml file (and struct)

dnephin · 2018-02-08T22:51:31Z

+		}
+		m["services"] = services
+	}
+	if c.Networks != nil {


Are these nil checks necessary (below)? If it is nil, it can assign nil without any problems, right? I think they can be removed.

dnephin · 2018-02-08T22:53:39Z

With full test looks good!

vdemeester · 2018-02-09T11:27:41Z

    ports:
      - 3000
-      - "3000-3005"
+      - "3001-3005"


Changed that to not have duplicate in the marshalled struct later.

vdemeester · 2018-02-09T11:30:43Z

@dnephin Updated with the full-example 🎉

dnephin · 2018-02-09T21:44:20Z

+		return nil, err
 	}
-
+	yaml := "version: \"" + version + "\"\n" + string(res)


Could we do this by marshaling a local struct:

type versionedConfig struct { composetypes.Config Version string }

thaJeztah

(removed: PBKAC)

thaJeztah · 2018-02-14T08:34:21Z

 		Args:    cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			opts.Namespace = args[0]
+


✂️ ✂️ ✂️ ✂️ ✂️ 😇

thaJeztah · 2018-02-14T09:55:53Z

+		fmt.Fprintf(dockerCli.Err(), "Ignoring deprecated options:\n\n%s\n\n",
+			propertyWarnings(deprecatedProperties))
+	}
+	return config, configDetails.Version, nil


Not for this PR, but getConfigDetails() actually loads all the config files, and takes the "WorkDir" and "Version" of the first file that's loaded. If we added a Version to composetypes.ConfigFile, that would probably be a bit cleaner, because in loader.Load() we check the version again (we both set the version if it's empty, and check if versions are mixed);

cli/cli/compose/loader/loader.go

Lines 52 to 59 in 1872bd8

configDict := file.Config

version := schema.Version(configDict)

if configDetails.Version == "" {

configDetails.Version = version

}

if configDetails.Version != version {

return nil, errors.Errorf("version mismatched between two composefiles : %v and %v", configDetails.Version, version)

}

Basically, these types;

cli/cli/compose/types/types.go

Lines 50 to 62 in 1872bd8

// ConfigFile is a filename and the contents of the file as a Dict

type ConfigFile struct {

Filename string

Config map[string]interface{}

}

// ConfigDetails are the details about a group of ConfigFiles

type ConfigDetails struct {

Version string

WorkingDir string

ConfigFiles []ConfigFile

Environment map[string]string

}

Would then look something like;

// ConfigFile is a filename and the contents of the file as a Dict type ConfigFile struct { Version string Filename string WorkingDir string Config map[string]interface{} } // ConfigDetails are the details about a group of ConfigFiles type ConfigDetails struct { ConfigFiles []ConfigFile Environment map[string]string }

(Perhaps there should be an option to overwrite the WorkingDir)

Which leads to the question if ConfigDetails on its own is very useful (Environment is currently just the result of os.Environ(), and could be either passed as separate argument to loader.Load(), or fetched there?)

thaJeztah · 2018-02-14T09:59:44Z

+		return details, errors.New("no composefile(s)")
+	}
+
+	if composefiles[0] == "-" && len(composefiles) == 1 {


We need to check if this behaviour is documented;

working-directory of the first compose-file is used

if the first file is from stdin, then the current working-directory is used

/cc @gbarr01

thaJeztah

one nit (unrelated whitespace change), and a suggestion for a follow up, but LGTM

To ensure we are loading the composefile the same wether we are pointing to swarm or kubernetes, we need to share the loading code between both. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

thaJeztah

LGTM, thanks!

thaJeztah · 2018-02-14T14:29:33Z

ping @dnephin still LGTY?

dnephin

LGTM

dnephin · 2018-02-14T16:51:53Z

+func TestMarshallConfig(t *testing.T) {
+	cfg := fullExampleConfig("/foo", "/bar")
+	expected := `configs: {}
+networks:


Hopefully MarshalYAML is stable and doesn't change order randomly when we change fields.

so far it did not 😇

silvin-lubecki

LGTM

[compose] Share the compose loading code between swarm and k8s stack deploy Upstream-commit: f56265a Component: cli

vdemeester requested review from dnephin, silvin-lubecki and thaJeztah January 29, 2018 21:23

GordonTheTurtle added the status/0-triage label Jan 29, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Jan 29, 2018

vdemeester force-pushed the stack-load-the-same branch from 0bb3d17 to 3169e97 Compare January 29, 2018 21:36

vdemeester commented Jan 29, 2018

View reviewed changes

silvin-lubecki reviewed Jan 29, 2018

View reviewed changes

vdemeester force-pushed the stack-load-the-same branch 2 times, most recently from a18de0b to d0f6ed5 Compare January 30, 2018 02:13

silvin-lubecki reviewed Jan 30, 2018

View reviewed changes

vdemeester force-pushed the stack-load-the-same branch 2 times, most recently from e1dbd5c to bd61f77 Compare January 31, 2018 23:57

vdemeester changed the title ~~WIP: Share the compose loading code between swarm and k8s stack deploy~~ [compose] Share the compose loading code between swarm and k8s stack deploy Jan 31, 2018

dnephin reviewed Feb 5, 2018

View reviewed changes

vdemeester force-pushed the stack-load-the-same branch 5 times, most recently from 1210082 to 6cb919f Compare February 8, 2018 16:41

dnephin reviewed Feb 8, 2018

View reviewed changes

vdemeester force-pushed the stack-load-the-same branch 2 times, most recently from eda38cc to ec92b3a Compare February 9, 2018 09:52

vdemeester force-pushed the stack-load-the-same branch from ec92b3a to 98e5148 Compare February 9, 2018 11:26

vdemeester commented Feb 9, 2018

View reviewed changes

dnephin reviewed Feb 9, 2018

View reviewed changes

vdemeester force-pushed the stack-load-the-same branch from 98e5148 to 1322abe Compare February 12, 2018 14:10

thaJeztah requested changes Feb 14, 2018

View reviewed changes

thaJeztah approved these changes Feb 14, 2018

View reviewed changes

Share the compose loading code between swarm and k8s stack deploy

570ee9c

To ensure we are loading the composefile the same wether we are pointing to swarm or kubernetes, we need to share the loading code between both. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the stack-load-the-same branch from 1322abe to 570ee9c Compare February 14, 2018 13:08

thaJeztah approved these changes Feb 14, 2018

View reviewed changes

dnephin approved these changes Feb 14, 2018

View reviewed changes

silvin-lubecki approved these changes Feb 19, 2018

View reviewed changes

vdemeester merged commit f56265a into docker:master Feb 19, 2018

GordonTheTurtle added this to the 18.03.0 milestone Feb 19, 2018

vdemeester deleted the stack-load-the-same branch February 19, 2018 14:28

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#845 from vdemeester/stack-load-the-same

157ffc1

[compose] Share the compose loading code between swarm and k8s stack deploy Upstream-commit: f56265a Component: cli

	configDict := file.Config
	version := schema.Version(configDict)
	if configDetails.Version == "" {
	configDetails.Version = version
	}
	if configDetails.Version != version {
	return nil, errors.Errorf("version mismatched between two composefiles : %v and %v", configDetails.Version, version)
	}

	// ConfigFile is a filename and the contents of the file as a Dict
	type ConfigFile struct {
	Filename string
	Config map[string]interface{}
	}

	// ConfigDetails are the details about a group of ConfigFiles
	type ConfigDetails struct {
	Version string
	WorkingDir string
	ConfigFiles []ConfigFile
	Environment map[string]string
	}

Conversation

vdemeester commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Feb 5, 2018

Uh oh!

dnephin commented Feb 5, 2018

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Feb 5, 2018

Uh oh!

vdemeester commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 14, 2018

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vdemeester commented Jan 29, 2018 •

edited

Loading

codecov-io commented Jan 29, 2018 •

edited

Loading

thaJeztah left a comment •

edited

Loading