Skip to content

Do not call the config endpoint if API is lower than 1.30 - #182

Merged
dnephin merged 1 commit into
docker:masterfrom
vdemeester:180-fix-stack-rm-config-endpoint
Jun 14, 2017
Merged

Do not call the config endpoint if API is lower than 1.30#182
dnephin merged 1 commit into
docker:masterfrom
vdemeester:180-fix-stack-rm-config-endpoint

Conversation

@vdemeester

@vdemeestervdemeester commented Jun 12, 2017

Copy link
Copy Markdown
Collaborator

Another would be to handle Page not found error in those cases 👼
(and moby/moby#33571 could help too 👼)

Fix#180

/cc @mavenugo@friism

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

Comment threadcli/command/stack/common.go Outdated
}
if versions.LessThan(version.APIVersion, "1.30") {
return []swarm.Config{}, nil
}

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.

I wonder if we should still print a warning (otherwise people may not be aware an option was ignored)

@vdemeestervdemeester added this to the 17.06.0 milestone Jun 12, 2017
@vdemeestervdemeester mentioned this pull request Jun 12, 2017
40 tasks
@vdemeester
vdemeesterforce-pushed the 180-fix-stack-rm-config-endpoint branch from 3ab7849 to fd7a10aCompareJune 12, 2017 16:37
@codecov-io

codecov-io commented Jun 12, 2017

Copy link
Copy Markdown

Codecov Report

Merging #182 into master will decrease coverage by 0.36%.
The diff coverage is 37.5%.

@@ Coverage Diff @@## master #182 +/- ##
==========================================
- Coverage 45.67% 45.31% -0.37% 
==========================================
Files 171 171 Lines 11505 11487 -18 ==========================================
- Hits 5255 5205 -50 - Misses 5946 5982 +36 + Partials 304 300 -4

@vdemeester
vdemeesterforce-pushed the 180-fix-stack-rm-config-endpoint branch from fd7a10a to 1c76877CompareJune 12, 2017 20:14
@thaJeztah

Copy link
Copy Markdown
Member

Perhaps make the warning more consistent with the error messages we have (and mention which API version is needed?) https://github.com/moby/moby/pull/33571/files#diff-c91a3260cd755746371aa100f9b599a6R24

Reading on my phone, haven checked if the docker-compose / stack deploy messages are different 😊

@vdemeester
vdemeesterforce-pushed the 180-fix-stack-rm-config-endpoint branch from 1c76877 to 99d2bdbCompareJune 13, 2017 09:32
Comment threadcli/command/stack/remove.go Outdated
return err
}
if versions.LessThan(version.APIVersion, "1.30") {
fmt.Fprintf(dockerCli.Err(), "WARNING: requires API version 1.30, but the Docker daemon API version is %s", version.APIVersion)

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.

What requires API version 1.30? If i saw this message from docker stack rm I would be confused. I think we need to include that this is about "configs" only.

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.

Oh, I was discussing options with @vdemeester this morning; guess I should've put them here, as he didn't come to updating yet 👍

@andrewhsuandrewhsu mentioned this pull request Jun 13, 2017
2 tasks
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester
vdemeesterforce-pushed the 180-fix-stack-rm-config-endpoint branch from 99d2bdb to 2128b3fCompareJune 14, 2017 07:57
@vdemeester

Copy link
Copy Markdown
CollaboratorAuthor

@dnephin@thaJeztah updated 👼

@cpuguy83cpuguy83 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

@dnephindnephin left a comment

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.

LGTM

@dnephin
dnephin merged commit 759e59e into docker:masterJun 14, 2017
@vdemeester
vdemeester deleted the 180-fix-stack-rm-config-endpoint branch June 14, 2017 18:34
@thaJeztah

Copy link
Copy Markdown
Member

oh! forgot to actually LGTM.. well, LGTM! thanks 😂

trapier pushed a commit to trapier/cli that referenced this pull request Sep 30, 2019
…-19.03-578ab52ece34b084621954e0b543003410cdbe84
[19.03] sync to upstream 19.03 578ab52
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
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.

6 participants

@vdemeester@codecov-io@thaJeztah@dnephin@cpuguy83@GordonTheTurtle