Add warnings client side before deployment on Kubernetes #903

silvin-lubecki · 2018-02-26T15:21:55Z

- What I did
I added a bunch of warnings for all the compose features unsupported by the Kubernetes Stack API. They are printed on stderr like this:

> $ docker stack deploy -c docker-compose.yml mystack
top-level network "postgres_conn" is ignored
service postgres: network "postgres_conn" is ignored
service postgres: env_file are ignored
service import-osmborder: network "postgres_conn" is ignored
service import-osmborder: env_file are ignored
service import-osm: network "postgres_conn" is ignored
service import-osm: env_file are ignored
service import-osm-diff: network "postgres_conn" is ignored
service import-osm-diff: env_file are ignored
service update-osm: network "postgres_conn" is ignored
service update-osm: env_file are ignored
service openmaptiles-tools: network "postgres_conn" is ignored
service openmaptiles-tools: env_file are ignored
service import-lakelines: network "postgres_conn" is ignored
service import-lakelines: env_file are ignored
service import-sql: network "postgres_conn" is ignored
service import-sql: env_file are ignored
service generate-changed-vectortiles: network "postgres_conn" is ignored
service generate-changed-vectortiles: env_file are ignored
service postserve: network "postgres_conn" is ignored
service postserve: env_file are ignored
service mapbox-studio: network "postgres_conn" is ignored
service import-wikidata: network "postgres_conn" is ignored
service import-wikidata: env_file are ignored
service generate-vectortiles: network "postgres_conn" is ignored
service generate-vectortiles: env_file are ignored
service import-water: network "postgres_conn" is ignored
service import-water: env_file are ignored
service import-natural-earth: network "postgres_conn" is ignored
service import-natural-earth: env_file are ignored
Waiting for the stack to be stable and running...

⚠️ Depends on #899 ⚠️

- How I did it
Once the compose file is parsed, I iterate on all services to detect the unsupported features (which are filled) and print a warning on each.

- How to verify it
Find a compose file with some networks and deploy it. Some warnings should be printed on stderr.

- Description for the changelog
Print warnings on stderr for each unsupported features while parsing a compose file for deployment on Kubernetes.

- A picture of a cute animal (not mandatory but encouraged)

codecov-io · 2018-02-26T15:28:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7ad3036). Click here to learn what that means.
The diff coverage is 36.77%.

@@            Coverage Diff            @@
##             master     #903   +/-   ##
=========================================
  Coverage          ?   53.37%           
=========================================
  Files             ?      267           
  Lines             ?    16975           
  Branches          ?        0           
=========================================
  Hits              ?     9060           
  Misses            ?     7322           
  Partials          ?      593

vdemeester

One nit, otherwise, one question : why doing that on its own instead of doing it while converting the types.Config to the stack struct ?

vdemeester · 2018-02-27T15:33:02Z

cli/command/stack/kubernetes/loader.go

 	"github.com/docker/cli/cli/compose/loader"
 	composetypes "github.com/docker/cli/cli/compose/types"
 	yaml "gopkg.in/yaml.v2"
 )

-func loadStack(name string, cfg composetypes.Config) (stack, error) {
+func loadStack(stderr io.Writer, name string, cfg *composetypes.Config) (stack, error) {


nit: any reason for having stderr as first argument ? 😝

For homogenization 😇 ?
https://github.com/docker/cli/blob/master/cli/command/stack/swarm/deploy_bundlefile.go#L100

silvin-lubecki · 2018-02-27T15:51:01Z

There are two reasons why I separated it. First one is for separation of concerns, as it's not a little 5 lines function. Second one is that if you put the warning code in the conversion code, then you may get warnings while you are retrieving some stacks, during a docker stack ls for example. I think we only want warnings on stacks you are deploying.

dnephin · 2018-02-27T20:02:05Z

cli/command/stack/kubernetes/warnings_test.go

+	var buf bytes.Buffer
+	warnUnsupportedFeatures(&buf, config)
+	warnings := buf.String()
+	assert.Contains(t, warnings, `top-level network "global" is ignored`)


Could this use a golden.Assert() for all the warnings, or is it possible for the order to change? Maybe we could sort the output if the order changes?

silvin-lubecki · 2018-03-01T14:35:10Z

PTAL

vdemeester

LGTM on commits that differs from #899

thaJeztah · 2018-04-26T13:43:02Z

Added [WIP] to the PR title until #899 is merged (reviewing that one)

thaJeztah

LGTM, but when rebasing can you quash the last two commits?

…a compose file for deployment on Kubernetes. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

mat007 · 2018-04-30T09:57:07Z

Rebase and squashing done.

thaJeztah · 2018-04-30T10:01:45Z

Thanks! Still LGTM

silvin-lubecki requested review from dnephin and vdemeester as code owners February 26, 2018 15:21

GordonTheTurtle added the status/0-triage label Feb 26, 2018

thaJeztah added the area/kubernetes label Feb 26, 2018

vdemeester added status/1-design-review and removed status/0-triage labels Feb 27, 2018

silvin-lubecki force-pushed the add-warnings-client-side branch from d470ec2 to ccd068d Compare February 27, 2018 15:30

vdemeester requested changes Feb 27, 2018

View reviewed changes

silvin-lubecki force-pushed the add-warnings-client-side branch from ccd068d to 897717d Compare February 27, 2018 15:44

dnephin reviewed Feb 27, 2018

View reviewed changes

silvin-lubecki force-pushed the add-warnings-client-side branch from 897717d to fd995d5 Compare February 28, 2018 23:58

silvin-lubecki force-pushed the add-warnings-client-side branch from fd995d5 to 8023cba Compare March 27, 2018 14:03

This was referenced Mar 27, 2018

Handle multiple stack api versions #899

Merged

Orchestrator column #973

Merged

vdemeester approved these changes Mar 28, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Mar 28, 2018

silvin-lubecki force-pushed the add-warnings-client-side branch from 8023cba to 87cce89 Compare March 30, 2018 13:08

thaJeztah changed the title ~~Add warnings client side before deployment on Kubernetes~~ [WIP] Add warnings client side before deployment on Kubernetes Apr 26, 2018

thaJeztah approved these changes Apr 30, 2018

View reviewed changes

Print warnings on stderr for each unsupported features while parsing …

36591a2

…a compose file for deployment on Kubernetes. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

mat007 force-pushed the add-warnings-client-side branch from 87cce89 to 36591a2 Compare April 30, 2018 09:56

thaJeztah changed the title ~~[WIP] Add warnings client side before deployment on Kubernetes~~ Add warnings client side before deployment on Kubernetes Apr 30, 2018

thaJeztah merged commit 8963ab9 into docker:master Apr 30, 2018

GordonTheTurtle added this to the 18.05.0 milestone Apr 30, 2018

thaJeztah modified the milestones: 18.05.0, 18.06.0 Apr 30, 2018

silvin-lubecki deleted the add-warnings-client-side branch May 18, 2018 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warnings client side before deployment on Kubernetes #903

Add warnings client side before deployment on Kubernetes #903

silvin-lubecki commented Feb 26, 2018

codecov-io commented Feb 26, 2018 •

edited

Loading

vdemeester left a comment

vdemeester Feb 27, 2018

silvin-lubecki Feb 27, 2018

vdemeester Feb 27, 2018

silvin-lubecki commented Feb 27, 2018

dnephin Feb 27, 2018

silvin-lubecki commented Mar 1, 2018

vdemeester left a comment

thaJeztah commented Apr 26, 2018

thaJeztah left a comment

mat007 commented Apr 30, 2018

thaJeztah commented Apr 30, 2018

Add warnings client side before deployment on Kubernetes #903

Add warnings client side before deployment on Kubernetes #903

Conversation

silvin-lubecki commented Feb 26, 2018

codecov-io commented Feb 26, 2018 • edited Loading

Codecov Report

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Feb 27, 2018

Choose a reason for hiding this comment

silvin-lubecki Feb 27, 2018

Choose a reason for hiding this comment

vdemeester Feb 27, 2018

Choose a reason for hiding this comment

silvin-lubecki commented Feb 27, 2018

dnephin Feb 27, 2018

Choose a reason for hiding this comment

silvin-lubecki commented Mar 1, 2018

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 26, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

mat007 commented Apr 30, 2018

thaJeztah commented Apr 30, 2018

codecov-io commented Feb 26, 2018 •

edited

Loading