Improve swagger.yaml to match the 1.41 api version #42621

gesellix · 2021-07-11T21:09:21Z

I recently tried to rely on the swagger.yaml for code generation of an api client. Some parts required manual fixes of the swagger docs to match the current api version 1.41.

All changes described below relate to a dedicated commit, because they aim at different parts of the api description and aren't releated per se. If you'd prefer to merge them all into a single commit, please leave a note.

- What I did

Add RestartPolicy "no" to swagger docs
Add "changes" query parameter for /image/create to swagger docs
Fix ContainerSummary swagger docs (flattened)
Use explicit object names for improved swagger based code generation (otherwise generic names had been generated)
Fix swagger docs to match the opencontainers image-spec

- How I did it

Used the 1.41 swagger.yaml with swagger's codegen to generate Java code. Used that code to perform requests to a 1.41 Docker engine. Applied some fixes and hack/validate/swagger and make swagger-docs to validate the changes.

- How to verify it

Compare the actual 1.41 api with the swagger.yaml.

- Description for the changelog

Update the swagger.yaml to match the version 1.41 api.

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

gesellix · 2021-07-11T21:22:44Z

Looks like I failed to actually validate the yaml file (the validate job fails):

[2021-07-11T21:19:57.490Z] The swagger spec at "api/swagger.yaml" is invalid against swagger specification 2.0. see errors :

[2021-07-11T21:19:57.490Z] - /containers/json in body must be of type object: "array"

thaJeztah

thanks! Changes look ok; I left a comment on how to fix the failing validation; if you can amend that commit to fix it

thaJeztah · 2021-07-12T09:55:35Z

api/swagger.yaml

-    type: "array"
-    items:


Looks like this definition is referenced in two places in the swagger;

/system/df -> responses.200.Containers which uses it as items in an array; so the swagger incorrectly defined it as an "array of arrays"

/containers/json -> responses -> 200 which used it directly, so an "array of objects"

I think your change makes sense; i.e. change ContainerSummary to an object (not an array of objects), so to make swagger validation pass, the /containers/json response needs to be updated.

If you amend the third commit ("Fix ContainerSummary swagger docs ") with the patch below, swagger validation should pass:

diff --git a/api/swagger.yaml b/api/swagger.yaml index fa71b38f09..efa2b81587 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -5261,7 +5261,9 @@ paths: 200: description: "no error" schema: - $ref: "#/definitions/ContainerSummary" + type: "array" + items: + $ref: "#/definitions/ContainerSummary" examples: application/json: - Id: "8dfafdbc3a40"

Thanks for your suggestion, looks perfect. I just applied your patch, let's see what the CI will find :)

relates to moby/moby#42621 (comment)

gesellix · 2021-07-12T10:28:12Z

Now the validation step reports a diff like this:

diff --git a/api/types/container/container_wait.go b/api/types/container/container_wait.go
index 49e05ae669..c2e69b5b25 100644
--- a/api/types/container/container_wait.go
+++ b/api/types/container/container_wait.go
@@ -6,14 +6,6 @@ package container // import "github.com/docker/docker/api/types/container"
 // See hack/generate-swagger-api.sh
 // ----------------------------------------------------------------------------
 
-// ContainerWaitOKBodyError container waiting error, if any
-// swagger:model ContainerWaitOKBodyError
-type ContainerWaitOKBodyError struct {
-
-	// Details of an error
-	Message string `json:"Message,omitempty"`
-}
-
 // ContainerWaitOKBody OK response to ContainerWait operation
 // swagger:model ContainerWaitOKBody
 type ContainerWaitOKBody struct {
@@ -26,3 +18,11 @@ type ContainerWaitOKBody struct {
 	// Required: true
 	StatusCode int64 `json:"StatusCode"`
 }
+
+// ContainerWaitOKBodyError container waiting error, if any
+// swagger:model ContainerWaitOKBodyError
+type ContainerWaitOKBodyError struct {
+
+	// Details of an error
+	Message string `json:"Message,omitempty"`
+}

This one looks like the struct has only moved down, but its content isn't changed. I added another commit to apply the codegen's changes.

gesellix · 2021-07-12T10:30:13Z

Will the CI retry automatically on errors like this?

ERROR: Get https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key: net/http: TLS handshake timeout

thaJeztah · 2021-07-12T14:33:56Z

This one looks like the struct has only moved down, but its content isn't changed. I added another commit to apply the codegen's changes.

Ah, dang, sorry, I should've mentioned.

That's flakiness in the code generator. For <reasons> (#42593), we're using an older version of go-swagger to generate the types. Unfortunately, that version uses a map[string]something internally, which (because go randomises map order) causes the order of some of those to be "random".

We know about that failure, so will just trigger CI again if it fails because of that (but should try to get rid of the old go-swagger version)

😓 could you remove the last commit again? 🤗

Will the CI retry automatically on errors like this?

Hmmm.. not it doesn't automatically; I noticed the same error earlier today on another PR (but restarting fixed it). Looks like there may be an issue with their package repository (or an issue with some of our CI nodes).

gesellix · 2021-07-12T14:38:10Z

Ah alright, yes, I've removed the last commit 😌

gesellix · 2021-07-12T14:39:56Z

ERROR: Get https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key: net/http: TLS handshake timeout happened again, maybe we should give OpenSuse some time to stabilize their servers.

thaJeztah · 2021-07-12T15:55:45Z

Ah alright, yes, I've removed the last commit 😌

Thank you! Sorry for that.

ERROR: Get https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key: net/http: TLS handshake timeout happened again, maybe we should give OpenSuse some time to stabilize their servers.

Yes. I see it succeeded on some machines, but failed on other machines.

Looking at the docker info output for a "succesful" and "failing" machine, both run the same version of docker and same Ubuntu version, so from that it doesn't appear to be something on our side.

[2021-07-12T14:38:31.437Z]  Server Version: 20.10.6
...
[2021-07-12T14:37:43.053Z]  Kernel Version: 5.4.0-1048-aws
[2021-07-12T14:37:43.053Z]  Operating System: Ubuntu 18.04.5 LTS

[2021-07-12T14:38:31.437Z]  Server Version: 20.10.6
...
[2021-07-12T14:38:31.437Z]  Kernel Version: 5.4.0-1048-aws
[2021-07-12T14:38:31.437Z]  Operating System: Ubuntu 18.04.5 LTS

Never say never of course; let me delete the failing Jenkins node (in case it's something on a specific machine), just to be sure.

thaJeztah

LGTM

to other reviewers; use ?w=1 to view the diff without the whitespace changes; https://github.com/moby/moby/pull/42621/files?w=1

thaJeztah · 2021-07-12T18:58:02Z

@AkihiroSuda @rvolosatovs ptal

rvolosatovs

LGTM

gesellix · 2021-07-18T15:42:13Z

Since I'm still working on an api client with model classes generated from the swagger.yaml, I could add more improvements similar to the forth commit (naming inlined objects so that generated code is more expressive). I can add the additional changes here or would you prefer me to create another pull request?

I also filed an issue to discuss another topic (nil objects and slices, marshalled as JSON). Maybe you can give me some feedback at #42632 so that I can have some kind of direction. This is also related to my efforts of improving the swagger.yaml - or the runtime api.

Edit: I added another commit ff35e40 with explicit type names and a consolidated PluginPrivilegeItem. I assume that the commit should be merged with 1e4704a, but I'll keep it until you had a chance for feedback/review.

gesellix · 2021-07-19T16:26:01Z

api/swagger.yaml

+      the plugin.
+    type: "object"
+    x-nullable: false
+    required: [Name, Value]


I hope adding the x-nullable: false and the required contrains don't break anything - or do they?

I think x-nullable: false would be the default, or is it not? If so, I guess it can be removed 🤔

Does it make sense to leave it for a separate PR? That could (a bit) simplify the review at least

I've removed the change and will create another PR.

gesellix · 2021-07-28T09:15:50Z

@thaJeztah you certainly have more important tasks to do, yet I wanted to ask if you could give some rough feedback on the recent changes. I'd like to know if this is a good direction or if there's anything to fix. The CI run doesn't appear to be related, but you might have more insights.

Like mentioned above: I'm working on a clean code generated api model, and the changes in this pull request are only the easy ones, while #42632 is probably more challenging due to a changed api (maybe). I would be glad if such kind of cleanup of the swagger docs and the api itself could be inlcuded in the upcoming v1.42 :)

gesellix · 2021-08-07T10:27:03Z

ping?

gesellix · 2021-08-16T17:37:03Z

@thaJeztah @AkihiroSuda @rvolosatovs would anyone like to have a look at the recent commit? I'd like to cleanup the pull request, so that it could get a final review.

rvolosatovs

Overall LGTM

rvolosatovs · 2021-08-17T17:55:47Z

api/swagger.yaml

@@ -2190,6 +2190,26 @@ definitions:
        type: "string"
        x-nullable: false

+  PluginPrivilegeItem:


Extraction of this type definition should probably better be done in a separate commit

rvolosatovs · 2021-08-17T17:58:41Z

api/swagger.yaml

@@ -7507,6 +7507,12 @@ paths:
            Refer to the [authentication section](#section/Authentication) for
            details.
          type: "string"
+        - name: "changes"
+          in: "query"
+          description: "Apply Dockerfile instruction to the created image."


It's not very clear to me what this does given the description

This one was taken from the CLI docs (see https://github.com/docker/cli/blob/c758c3e4a5a980cf0ea3292c958fd537822ba0d5/docs/reference/commandline/import.md#description), trying to keep some consistency. Yet, the cli docs are more specific regarding the supported Dockerfile commands.

I changed the description to be more detailed.

rvolosatovs · 2021-08-17T18:00:32Z

api/swagger.yaml

+      the plugin.
+    type: "object"
+    x-nullable: false
+    required: [Name, Value]


Does it make sense to leave it for a separate PR? That could (a bit) simplify the review at least

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>

gesellix · 2021-08-19T19:19:01Z

@rvolosatovs From my perspective this one is ready for a final review. Please leave a suggestion if the description for /images/create?changes=... is not clear enough, yet. Thanks!

gesellix · 2021-08-20T06:56:32Z

The failing CI checks don't seem to be related to my changes, or do they?

thaJeztah

arf, I see I was writing a review, but didn't submit it.

Changes LGTM, thanks!

I see some of my comments are now outdated, and I was working on some other changes, so let me get this one merged.

thaJeztah · 2021-08-09T10:03:51Z

api/swagger.yaml

+      the plugin.
+    type: "object"
+    x-nullable: false
+    required: [Name, Value]


I think x-nullable: false would be the default, or is it not? If so, I guess it can be removed 🤔

api/swagger.yaml

thaJeztah · 2021-08-21T19:17:24Z

api/swagger.yaml

                      type: "array"
                      items:
                        type: "string"
-                    Variant:
+                    variant:
                      type: "string"
                    Features:


Looks like this one can be removed now.

I was working on some other changes, and can include that in a follow-up

gesellix · 2021-08-22T13:34:52Z

Thanks for merging! I'm still not finished with the overall improvement for the swagger.yaml, but the upcoming changes might be harder/riskier than the ones in this PR.

gesellix force-pushed the swagger-wip branch from 395a81b to 06faf7d Compare July 11, 2021 21:14

gesellix mentioned this pull request Jul 11, 2021

Propose swagger.yaml modifications upstream docker-client/docker-engine#62

Open

2 tasks

thaJeztah added area/api status/2-code-review area/docs labels Jul 12, 2021

thaJeztah requested changes Jul 12, 2021

View reviewed changes

gesellix force-pushed the swagger-wip branch from 06faf7d to 2894a1c Compare July 12, 2021 10:09

gesellix added a commit to docker-client/docker-engine that referenced this pull request Jul 12, 2021

fix validation errors

4d485e0

relates to moby/moby#42621 (comment)

gesellix force-pushed the swagger-wip branch from 50ab1f4 to 2894a1c Compare July 12, 2021 14:37

thaJeztah approved these changes Jul 12, 2021

View reviewed changes

rvolosatovs approved these changes Jul 13, 2021

View reviewed changes

gesellix commented Jul 19, 2021

View reviewed changes

gesellix force-pushed the swagger-wip branch from 1e712b6 to 857dfcb Compare July 19, 2021 18:43

gesellix requested a review from thaJeztah July 26, 2021 07:56

gesellix force-pushed the swagger-wip branch from 857dfcb to ff35e40 Compare August 1, 2021 20:09

rvolosatovs reviewed Aug 17, 2021

View reviewed changes

gesellix force-pushed the swagger-wip branch from ff35e40 to 49e5613 Compare August 19, 2021 18:32

Add RestartPolicy "no" to swagger docs

d741c14

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>

gesellix force-pushed the swagger-wip branch from 49e5613 to cc915e7 Compare August 19, 2021 18:33

gesellix added 5 commits August 19, 2021 21:10

Add "changes" query parameter for /image/create to swagger docs

6b53f2c

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>

Fix ContainerSummary swagger docs

9275268

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>

Use explicit object names for improved swagger based code generation

52a9f16

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>

Extract PluginPrivilegeItem as explicit type definition

ef44b22

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>

Fix swagger docs to match the opencontainers image-spec

8f75d8d

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>

gesellix force-pushed the swagger-wip branch from cc915e7 to 8f75d8d Compare August 19, 2021 19:10

thaJeztah added this to the 21.xx milestone Aug 21, 2021

thaJeztah added the process/cherry-pick label Aug 21, 2021

thaJeztah approved these changes Aug 21, 2021

View reviewed changes

thaJeztah merged commit 9bc0c49 into moby:master Aug 21, 2021

thaJeztah mentioned this pull request Aug 21, 2021

swagger: assorted fixes and updates #42769

Merged

gesellix deleted the swagger-wip branch August 22, 2021 09:17

gesellix added a commit to docker-client/docker-engine that referenced this pull request Aug 22, 2021

rebase partially to moby/moby#42621

6c6d551

gesellix added a commit to docker-client/docker-engine that referenced this pull request Aug 22, 2021

rebase partially to moby/moby#42621

8bce782

This was referenced Jan 20, 2022

docs: update API v1.41 and v1.40 docs with fixes from api/swagger #43170

Merged

[20.10 backport] docs: update API v1.41 and v1.40 docs with fixes from api/swagger #43171

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jan 20, 2022

thaJeztah mentioned this pull request May 23, 2022

Clean-up container disk usage output #42711

Open

thaJeztah mentioned this pull request Jan 10, 2023

docs: api: synchronise versioned API docs (v1.39 - v1.42) #44784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve swagger.yaml to match the 1.41 api version #42621

Improve swagger.yaml to match the 1.41 api version #42621

gesellix commented Jul 11, 2021

gesellix commented Jul 11, 2021

thaJeztah left a comment

thaJeztah Jul 12, 2021

gesellix Jul 12, 2021

gesellix commented Jul 12, 2021

gesellix commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

gesellix commented Jul 12, 2021

gesellix commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

thaJeztah left a comment

thaJeztah commented Jul 12, 2021

rvolosatovs left a comment

gesellix commented Jul 18, 2021 •

edited

Loading

gesellix Jul 19, 2021

thaJeztah Aug 9, 2021

rvolosatovs Aug 17, 2021

gesellix Aug 19, 2021

gesellix commented Jul 28, 2021

gesellix commented Aug 7, 2021

gesellix commented Aug 16, 2021

rvolosatovs left a comment

rvolosatovs Aug 17, 2021

rvolosatovs Aug 17, 2021

gesellix Aug 19, 2021

rvolosatovs Aug 17, 2021

gesellix commented Aug 19, 2021

gesellix commented Aug 20, 2021

thaJeztah left a comment

thaJeztah Aug 9, 2021

thaJeztah Aug 21, 2021

gesellix commented Aug 22, 2021

Improve swagger.yaml to match the 1.41 api version #42621

Improve swagger.yaml to match the 1.41 api version #42621

Conversation

gesellix commented Jul 11, 2021

gesellix commented Jul 11, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesellix commented Jul 12, 2021

gesellix commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

gesellix commented Jul 12, 2021

gesellix commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Jul 12, 2021

rvolosatovs left a comment

Choose a reason for hiding this comment

gesellix commented Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesellix commented Jul 28, 2021

gesellix commented Aug 7, 2021

gesellix commented Aug 16, 2021

rvolosatovs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesellix commented Aug 19, 2021

gesellix commented Aug 20, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesellix commented Aug 22, 2021

gesellix commented Jul 18, 2021 •

edited

Loading