Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotate some swagger documented types as int64 #43629

Merged
merged 1 commit into from
May 25, 2022

Conversation

gesellix
Copy link
Contributor

@gesellix gesellix commented May 22, 2022

Using the swagger.yaml to generate api models will create incompatible field types. Some inconsistencies had already been mentioned at #39131. I've added more fixes from real life experience, some only occurring on Windows.

Closes #39131

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

- What I did

The following types documented in api/swagger.yaml and docs/api/v1.41.yaml have been adjusted to match the types in the Golang model:

- How I did it

Changed the swagger.yaml and v1.41.yaml, compared with moby's Golang types (didn't check any older api versions).

- How to verify it

See the list above, with each entry linking to the Golang source for easier comparison.

- Description for the changelog

Annotate some Swagger documented types as int64, matching the Golang types.

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

ladybug

@gesellix
Copy link
Contributor Author

Looks like I forgot to update the types/image_summary.go for the fixed typo (s/imagem/image,/), I'm going to fix that soon.

@thaJeztah
Copy link
Member

Thanks!

Hmm.. validate is failing 🤔 ;

The result of hack/generate-swagger-api.sh differs
 
diff --git a/api/types/image_summary.go b/api/types/image_summary.go
index 53c2885758..90b983a25c 100644
--- a/api/types/image_summary.go
+++ b/api/types/image_summary.go
@@ -61,7 +61,7 @@ type ImageSummary struct {
 	// List of image names/tags in the local image cache that reference this
 	// image.
 	//
-	// Multiple image tags can refer to the same imagem and this list may be
+	// Multiple image tags can refer to the same image, and this list may be
 	// empty if no tags reference the image, in which case the image is
 	// "untagged", in which case it can still be referenced by its ID.
 	//

Please update api/swagger.yaml with any API changes, then 
run hack/generate-swagger-api.sh.

Ah! I think that's because that bit is related to the Go types;

// Multiple image tags can refer to the same imagem and this list may be

and

// Multiple image tags can refer to the same imagem and this list may be

So those will have to be updated as well 😅

@thaJeztah
Copy link
Member

Looks like I forgot to update the types/image_summary.go for the fixed typo (s/imagem/image,/), I'm going to fix that soon.

Whoops; I had the tab open with my comment in draft, and didn't see your post 😂

@gesellix
Copy link
Contributor Author

gesellix commented May 23, 2022

@thaJeztah thanks for pointing me to the second occurrence at

// Multiple image tags can refer to the same imagem and this list may be
:)

Using the swagger.yaml to generate api models will create incompatible field types. Some inconsistencies had already been mentioned at moby#39131. I've added more fixes from real life experience, some only occurring on Windows.

Closes moby#39131

Signed-off-by: Tobias Gesellchen <tobias@gesellix.de>
@gesellix
Copy link
Contributor Author

Wow... "Upstream fix" somewhere far away closed this pull request. TIL.

@gesellix
Copy link
Contributor Author

The failing test case doesn't seem to be related to my changes 🤔

[2022-05-23T20:28:57.914Z] === Failed
[2022-05-23T20:28:57.914Z] === FAIL: amd64.integration-cli TestDockerSuite/TestExecWithPrivileged (0.83s)
[2022-05-23T20:28:57.914Z]     docker_cli_exec_test.go:463: assertion failed: 
[2022-05-23T20:28:57.914Z]         Command:  /usr/local/cli/docker logs parent
[2022-05-23T20:28:57.914Z]         ExitCode: 1
[2022-05-23T20:28:57.914Z]         Error:    exit status 1
[2022-05-23T20:28:57.914Z]         Stdout:   Privileged exec has not run yet
[2022-05-23T20:28:57.914Z]         Privileged exec has not run yet
[2022-05-23T20:28:57.914Z]         Privileged exec has not run yet
[2022-05-23T20:28:57.914Z]         
[2022-05-23T20:28:57.914Z]         Stderr:   error from daemon in stream: Error grabbing logs: invalid character 'l' after object key:value pair
[2022-05-23T20:28:57.914Z]         
[2022-05-23T20:28:57.914Z]         
[2022-05-23T20:28:57.914Z]         
[2022-05-23T20:28:57.914Z]         Failures:
[2022-05-23T20:28:57.914Z]         ExitCode was 1 expected 0
[2022-05-23T20:28:57.914Z]         Expected no error
[2022-05-23T20:28:57.914Z]     --- FAIL: TestDockerSuite/TestExecWithPrivileged (0.83s)
[2022-05-23T20:28:57.914Z] 
[2022-05-23T20:28:57.914Z] === FAIL: amd64.integration-cli TestDockerSuite (1370.50s)
[2022-05-23T20:28:57.914Z] 

@thaJeztah
Copy link
Member

Wow... "Upstream fix" somewhere far away closed this pull request. TIL.

Ah, yes, those can be fun; even more so if the fix #xyz is in the commit message (can't edit those after the fact)

The failing test case doesn't seem to be related to my changes

Probably some flakiness; let me kick CI again

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah added this to the 22.06.0 milestone May 24, 2022
@gesellix
Copy link
Contributor Author

gesellix commented May 25, 2022

This one also doesn't seem to be related:

[2022-05-24T12:59:49.289Z] === FAIL: github.com/docker/docker/integration-cli TestDockerSuite/TestRunContainerWithRmFlagCannotStartContainer (2.20s)
[2022-05-24T12:59:49.289Z]     docker_cli_run_test.go:2770: Expected not to have containers f11f42a8a90e
[2022-05-24T12:59:49.289Z]         
[2022-05-24T12:59:49.289Z]     --- FAIL: TestDockerSuite/TestRunContainerWithRmFlagCannotStartContainer (2.20s)
[2022-05-24T12:59:49.289Z] 
[2022-05-24T12:59:49.289Z] === FAIL: github.com/docker/docker/integration-cli TestDockerSuite (2660.72s)
[2022-05-24T12:59:49.289Z] 

If I can do anything to make it work, please leave a note here :)

@thaJeztah
Copy link
Member

Looks like that test was flaky before; #11966 (and I saw a couple of other mentions). Should not be related, but let me kick it once more just to see if we can make it nice and green 😅

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #43629 (345346d) into master (0a3767c) will decrease coverage by 5.13%.
The diff coverage is 61.44%.

@@            Coverage Diff             @@
##           master   #43629      +/-   ##
==========================================
- Coverage   36.18%   31.04%   -5.14%     
==========================================
  Files         610      643      +33     
  Lines       45363    65896   +20533     
==========================================
+ Hits        16413    20456    +4043     
- Misses      26707    43539   +16832     
+ Partials     2243     1901     -342     

@tianon tianon merged commit c0069b8 into moby:master May 25, 2022
@gesellix gesellix deleted the int64-fields branch May 25, 2022 19:42
Sign up for free to 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.

Swagger spec for HealthConfig type's time.Duration fields does not annotate them as 64-bit ints
3 participants