Return warnings from service create/update when digest pinning fails by aaronlehmann · Pull Request #28421 · moby/moby

aaronlehmann · 2016-11-15T02:13:45Z

Modify the service update and create APIs to return optional warning
messages as part of the response. Populate these messages with an
informative reason when digest resolution fails.

This is a small API change, but significantly improves the UX. The user
can now get immediate feedback when they've specified a nonexistent
image or unreachable registry.

cc @nishanttotla @dnephin

vieux · 2016-11-15T02:19:14Z

@aaronlehmann can you please update swagger and use it to generate types ? /cc @dnephin

aaronlehmann · 2016-11-15T05:02:14Z

@vieux: Can you please point me to some information on how to do that? ServiceCreateResponse already exists in client.go, so adding ServiceUpdateResponse next to it seemed like the right thing to do.

nishanttotla · 2016-11-15T07:46:57Z

api/types/client.go

It seems like the only use case for the Warning right now is a failure to pin by digest. Should we make it a general string field then, as opposed to maybe a boolean or something else?

Not in favor of a boolean because it's important to communicate the actual error message. I think a general string field is necessary. However, we could add a boolean in addition to indicate whether pinning by digest happened if people think it's a good idea.

I don't think another field is necessary in addition to a string Warning field, particularly because we're not taking any specific action if the pinning by digest fails.

thaJeztah · 2016-11-15T12:46:41Z

ping @dnephin can you help with the Swagger changes?

dnephin · 2016-11-15T15:19:21Z

@aaronlehmann

Update api/swagger.yaml. At the same time change operationId: "PostServicesUpdate" to ServiceUpdate. Also change tags: - Services to tags: [Service] I've been normalizing as I go.
Add -n ServiceUpdate to hack/generate-swagger-api.sh
Run make swagger-gen (or hack/generate-swagger-api.sh directly from make shell)
That should produce a new file at api/types/service
Change the imports to use that struct

aaronlehmann · 2016-11-15T17:40:44Z

@dnephin: Can you confirm that it's correct to generate ServiceUpdateResponse from swagger even though ServiceCreateResponse does not seem to be generated that way?

dnephin · 2016-11-15T17:46:03Z

Yes, that's correct. We're slowly migrating everything to the swagger generation in #27919. It would be good to any any new things using the new pattern.

aaronlehmann · 2016-11-15T18:45:02Z

@dnephin: I've made some progress but am stuck on a few things.

Currently I'm getting this output:

// ServiceUpdateOKBody service update o k body
// swagger:model ServiceUpdateOKBody
type ServiceUpdateOKBody struct {

       // Optional warning message
       // Required: true
       Warning string `json:"Warning"`
}

The Required: true is false, and it's added automatically. I haven't found any way to suppress this. Adding required: [] under schema does not change anything. Adding required: false under Warning causes the structure not to be generated at all.
How can I add the omitempty property?

aaronlehmann · 2016-11-15T19:12:03Z

I think I figured it out.

PTAL

dnephin

LGTM

aaronlehmann · 2016-11-15T22:12:20Z

Just occurred to me - should Warning be an array called Warnings to be consistent with ContainerCreateCreatedBody?

thaJeztah · 2016-11-16T00:58:05Z

@aaronlehmann should this be considered for 1.13?

aaronlehmann · 2016-11-16T01:49:00Z

Yes please.

aaronlehmann · 2016-11-16T22:43:27Z

Updated to return an array of warnings. This matches other API endpoints like /containers/create.

PTAL

thaJeztah · 2016-11-16T23:33:33Z

docs/reference/api/docker_remote_api_v1.25.md

don't forget to add a bullet to the API changes; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v125-api-changes

Also; should this change be versioned (and only return warnings on API >= 1.25

I always forget.

Done.

re versioning: I'm not sure what our normal policy is on this kind of change, but it seems like a lot of extra complexity to do this unless we have a good reason. I expect that older clients would just ignore this field.

Hm, you may be right here; we document that additional fields may be returned by the API, so clients should ignore them.

aaronlehmann · 2016-11-17T20:46:34Z

Updatd docker_remote_api_v1.26.md as well.

PTAL

thaJeztah · 2016-11-17T23:10:46Z

(docs LGTM)

vdemeester · 2016-11-18T11:23:23Z

@aaronlehmann needs a rebase 😓

…nning fails Modify the service update and create APIs to return optional warning messages as part of the response. Populate these messages with an informative reason when digest resolution fails. This is a small API change, but significantly improves the UX. The user can now get immediate feedback when they've specified a nonexistent image or unreachable registry. Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann · 2016-11-18T17:31:41Z

Rebased

vdemeester

LGTM (code & docs) 🐮

GordonTheTurtle added the status/0-triage label Nov 15, 2016

aaronlehmann added status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Nov 15, 2016

nishanttotla reviewed Nov 15, 2016

View reviewed changes

thaJeztah added the status/1-design-review label Nov 15, 2016

aaronlehmann force-pushed the digest-pinning-warnings branch from acbdad9 to ccfa9bc Compare November 15, 2016 17:42

aaronlehmann force-pushed the digest-pinning-warnings branch from ccfa9bc to f0d2912 Compare November 15, 2016 19:11

nishanttotla mentioned this pull request Nov 15, 2016

Pin images by digest #28173

Merged

aaronlehmann force-pushed the digest-pinning-warnings branch from f0d2912 to 252994e Compare November 15, 2016 20:46

aaronlehmann mentioned this pull request Nov 15, 2016

Docker swarm infinitely attempts to pull non-existent image, even after service removed #28210

Open

dnephin approved these changes Nov 15, 2016

View reviewed changes

aaronlehmann removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 15, 2016

aaronlehmann force-pushed the digest-pinning-warnings branch from 252994e to a77a3a4 Compare November 16, 2016 22:42

thaJeztah added this to the 1.13.0 milestone Nov 16, 2016

thaJeztah added process/cherry-pick impact/api labels Nov 16, 2016

thaJeztah reviewed Nov 16, 2016

View reviewed changes

aaronlehmann force-pushed the digest-pinning-warnings branch from a77a3a4 to b63d861 Compare November 16, 2016 23:37

thaJeztah added status/2-code-review and removed status/1-design-review labels Nov 17, 2016

vdemeester added the rebuild/windowsRS1 label Nov 17, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Nov 17, 2016

aaronlehmann force-pushed the digest-pinning-warnings branch from b63d861 to 5e6adf6 Compare November 17, 2016 20:45

aaronlehmann force-pushed the digest-pinning-warnings branch from 5e6adf6 to 948e606 Compare November 18, 2016 17:31

vdemeester approved these changes Nov 18, 2016

View reviewed changes

vdemeester added status/4-merge and removed status/2-code-review labels Nov 18, 2016

aaronlehmann merged commit 30d0c38 into moby:master Nov 18, 2016

aaronlehmann deleted the digest-pinning-warnings branch November 18, 2016 19:33

vieux mentioned this pull request Nov 18, 2016

1.13.0-rc2 cherry-picks : part 2 #28489

Merged

vieux added process/cherry-picked and removed process/cherry-pick labels Nov 18, 2016

nishanttotla mentioned this pull request Dec 7, 2016

Unclear error messages creating Swarm service with image from local library #29205

Closed

Conversation

aaronlehmann commented Nov 15, 2016

Uh oh!

vieux commented Nov 15, 2016

Uh oh!

aaronlehmann commented Nov 15, 2016

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 Nov 15, 2016

Uh oh!

dnephin commented Nov 15, 2016

Uh oh!

aaronlehmann commented Nov 15, 2016

Uh oh!

dnephin commented Nov 15, 2016

Uh oh!

aaronlehmann commented Nov 15, 2016

Uh oh!

aaronlehmann commented Nov 15, 2016

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Nov 15, 2016

Uh oh!

thaJeztah commented Nov 16, 2016

Uh oh!

aaronlehmann commented Nov 16, 2016

Uh oh!

aaronlehmann commented Nov 16, 2016

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!

aaronlehmann commented Nov 17, 2016

Uh oh!

thaJeztah commented Nov 17, 2016

Uh oh!

vdemeester commented Nov 18, 2016

Uh oh!

aaronlehmann commented Nov 18, 2016

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants