Skip to content

Add warnings client side before deployment on Kubernetes - #903

Merged
thaJeztah merged 1 commit into
docker:masterfrom
silvin-lubecki:add-warnings-client-side
Apr 30, 2018
Merged

Add warnings client side before deployment on Kubernetes#903
thaJeztah merged 1 commit into
docker:masterfrom
silvin-lubecki:add-warnings-client-side

Conversation

@silvin-lubecki

Copy link
Copy Markdown
Contributor

- 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)
image

@codecov-io

codecov-io commented Feb 26, 2018

Copy link
Copy Markdown

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

@vdemeestervdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

)

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@silvin-lubecki

Copy link
Copy Markdown
ContributorAuthor

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.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

PTAL

@silvin-lubecki
silvin-lubeckiforce-pushed the add-warnings-client-side branch from fd995d5 to 8023cbaCompareMarch 27, 2018 14:03
This was referenced Mar 27, 2018

@vdemeestervdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM on commits that differs from #899

@silvin-lubecki
silvin-lubeckiforce-pushed the add-warnings-client-side branch from 8023cba to 87cce89CompareMarch 30, 2018 13:08
@thaJeztahthaJeztah changed the title Add warnings client side before deployment on Kubernetes[WIP] Add warnings client side before deployment on KubernetesApr 26, 2018
@thaJeztah

Copy link
Copy Markdown
Member

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

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
mat007force-pushed the add-warnings-client-side branch from 87cce89 to 36591a2CompareApril 30, 2018 09:56
@mat007

Copy link
Copy Markdown
Member

Rebase and squashing done.

@thaJeztahthaJeztah changed the title [WIP] Add warnings client side before deployment on KubernetesAdd warnings client side before deployment on KubernetesApr 30, 2018
@thaJeztah

Copy link
Copy Markdown
Member

Thanks! Still LGTM

@thaJeztah
thaJeztah merged commit 8963ab9 into docker:masterApr 30, 2018
@GordonTheTurtleGordonTheTurtle added this to the 18.05.0 milestone Apr 30, 2018
@thaJeztahthaJeztah modified the milestones: 18.05.0, 18.06.0Apr 30, 2018
@silvin-lubecki
silvin-lubecki deleted the add-warnings-client-side branch May 18, 2018 12:27
Sign up for freeto 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.

7 participants

@silvin-lubecki@codecov-io@thaJeztah@mat007@vdemeester@dnephin@GordonTheTurtle