Don't attempt to remove unsupported resources on older daemon by thaJeztah · Pull Request #277 · docker/cli

thaJeztah · 2017-07-01T00:13:19Z

When running docker stack rm <some stack> against an older daemon, a warning was printed for "configs" being ignored;

WARNING: ignoring "configs" (requires API version 1.30, but the Docker daemon API version is 1.26)

Given that an old daemon cannot have configs, there should not be a need to warn, or attempt to remove these resources.

This patch removes the warning, and skips removing configs.

A check if secrets are supported by the daemon is also added, given that this would result in an error when attempted against an older (pre 1.13) daemon.

There is one situation where this could lead to secrets or configs being left behind; if the client is connecting to a daemon that does support secrets, configs, but the API version is overridden using DOCKER_API_VERSION, no warning is printed, and secrets and configs are not attempted to be removed.

Given that DOCKER_API_VERSION is regarded a feature for debugging / "power users", it should be ok to ignore this.

ping @dnephin @vdemeester @briantd

codecov-io · 2017-07-01T00:22:31Z

Codecov Report

Merging #277 into master will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@ Coverage Diff @@## master #277 +/- ##
=========================================
+ Coverage 48.68% 48.7% +0.01% 
=========================================
Files 186 186 Lines 12416 12413 -3 =========================================
+ Hits 6045 6046 +1 + Misses 5996 5995 -1 + Partials 375 372 -3

thaJeztah · 2017-07-01T07:30:38Z

Actually, I realised this is what we had originally in #182, but I failed to see that we only called these endpoints on removal, not in other cases 😓

dnephin · 2017-07-04T15:29:43Z

+ errs []string
+ secrets []swarm.Secret
+ configs []swarm.Config
+	)


This is a minor nit, but I think it's generally better to keep variable declarations closer to the code where they are used. It makes it easier to see the scope of the variable and to refactor the code later.
So these could move to immediately before the if versions... check for each

Updated; was thinking that declaring the variables inside the loop was bad, but reading up, Go is smart enough to handle that 👍

dnephin · 2017-07-04T15:32:11Z

+ configs []swarm.Config
+	)
+
+	version, err := client.ServerVersion(ctx)


I missed this in the earlier PR. I think we should already have version stored, we shouldn't need to make another API call.
It's available from cli.Client().ClientVersion(). The name is misleading, but this version was already set from the server version.

Ok, so I tried using this, but ran into issues. the ClientVersion() is returning cli.version
cli.version is only set by NegotiateAPIVersionPing(), and NewClient(), both of which are not used for our fakeClient.
I decided to use the "old" approach for now, but we should look into this, as it's only an issue for testing
Perhaps you have a trick up your sleeve to make it work though 😄

dnephin · 2017-07-04T15:33:51Z

+ if versions.GreaterThanOrEqualTo(version.APIVersion, "1.25") {
+ hasError = removeSecrets(ctx, dockerCli, secrets) || hasError
+ }
+ if versions.GreaterThanOrEqualTo(version.APIVersion, "1.30") {


These two conditions aren't necessary because removeSecrets() and removeConfigs() iterate over the array, so it's already a no-op if they are empty.

thaJeztah · 2017-07-05T03:59:12Z

@dnephin @vdemeester updated PTAL

dnephin · 2017-07-05T14:50:43Z

test failures

thaJeztah · 2017-07-10T23:43:36Z

Tests are passing again, see my comments above 👍

dnephin · 2017-07-11T16:23:23Z

 	client := dockerCli.Client()
 	ctx := context.Background()
-
+	version, err := client.ServerVersion(ctx)


~You can set ClientVersion() to the version you want like this:~~
&fakeClient{ Client: client.NewClient("http://example.com", apiVersion, nil, nil), ... }
Edit: Or an even better option would be to add ClientVersion() and version string to fakeClient{} and have it return that version.

I think I tried adding version and ClientVersion() to fakeClient{}, but it didn't satisfy the Interface. Hm, let me check, perhaps I didn't

Oh, I know what I did 🤦‍♂️ - yes this works, thanks! Adding some additional test-cases for different versions now

When running `docker stack rm <some stack>` against an older daemon, a warning was printed for "configs" being ignored; WARNING: ignoring "configs" (requires API version 1.30, but the Docker daemon API version is 1.26) Given that an old daemon cannot _have_ configs, there should not be a need to warn, or _attempt_ to remove these resources. This patch removes the warning, and skips fetching (and removing) configs. A check if _secrets_ are supported by the daemon is also added, given that this would result in an error when attempted against an older (pre 1.13) daemon. There is one situation where this could lead to secrets or configs being left behind; if the client is connecting to a daemon that _does_ support secrets, configs, but the API version is overridden using `DOCKER_API_VERSION`, no warning is printed, and secrets and configs are not attempted to be removed. Given that `DOCKER_API_VERSION` is regarded a feature for debugging / "power users", it should be ok to ignore this. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

dnephin

LGTM

dnephin · 2017-07-11T17:56:12Z

+	assert.NoError(t, cmd.Execute())
+	assert.Equal(t, allServiceIDs, cli.removedServices)
+	assert.Equal(t, allNetworkIDs, cli.removedNetworks)
+	assert.Nil(t, cli.removedSecrets)


I generally prefer to make these separate test cases instead of inline comments. It's never really clear where an inline comment ends. With a separate test function the scope is obvious.
Any shared setup can be moved into a helper function to remove the duplication, which also helps make the test more readable.
This is a minor style thing, so not blocking.

vdemeester

LGTM 🐸
Agreeing with @dnephin on the separate tests though 👼

thaJeztah · 2017-07-12T08:22:59Z

yes; at first was doubting to make it a test-table, or separate tests, but kept it like this; I can update if you want, or do a follow up. If we want this into 17.06.1, perhaps a follow up

vdemeester · 2017-07-12T08:43:51Z

@thaJeztah counting on you for the follow-up 👼 🤗

thaJeztah added the status/2-code-review label Jul 1, 2017

GordonTheTurtle added the status/0-triage label Jul 1, 2017

thaJeztah removed the status/0-triage label Jul 1, 2017

dnephin reviewed Jul 4, 2017
View reviewed changes

thaJeztah force-pushed the dont-remove-what-isnt-there branch 2 times, most recently from 813ae2f to 87cb17aCompare July 5, 2017 03:58

thaJeztah force-pushed the dont-remove-what-isnt-there branch from 87cb17a to ac5093cCompare July 10, 2017 23:28

dnephin suggested changes Jul 11, 2017
View reviewed changes

thaJeztah force-pushed the dont-remove-what-isnt-there branch from ac5093c to 2863ee2Compare July 11, 2017 17:27

thaJeztah force-pushed the dont-remove-what-isnt-there branch from 2863ee2 to 2429f15Compare July 11, 2017 17:29

dnephin approved these changes Jul 11, 2017
View reviewed changes

vdemeester approved these changes Jul 12, 2017
View reviewed changes

vdemeester merged commit 87345ed into docker:masterJul 12, 2017

GordonTheTurtle added this to the 17.07.0 milestone Jul 12, 2017

dnephin mentioned this pull request Jul 12, 2017
Fix test and lint failures on master #326
Merged

thaJeztah deleted the dont-remove-what-isnt-there branch August 1, 2017 14:00

Uh oh!

Conversation

thaJeztah commented Jul 1, 2017

Uh oh!

codecov-io commented Jul 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Jul 1, 2017

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!

thaJeztah commented Jul 5, 2017

Uh oh!

dnephin commented Jul 5, 2017

Uh oh!

thaJeztah commented Jul 10, 2017

Uh oh!

dnephinJul 11, 2017• 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!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 12, 2017

Uh oh!

vdemeester commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jul 1, 2017•
edited
Loading

dnephinJul 11, 2017•
edited
Loading