api: image inspect: add back fields that did not omitempty #50135
Conversation
Probably; I also just came to the conclusion that we can combine the existing fallback with the new one, and (AFAICS) that would also mean we can drop the I have a commit doing so; let me push that, and I can squash it iafter that. |
|
OK; pushed a commit; let me know what you think, then I can squash. |
|
Looks like I broke something in that last commit; will have a look tomorrow |
585aadb to
4ff4fa2
Compare
| // prevent mutating legacyConfigFields. | ||
| cfg := maps.Clone(ir.legacyConfig) |
There was a problem hiding this comment.
This was the problem; I added the legacyConfigFields the router assigns legacyConfig from that, and we didn't de-reference.
| imageInspect := &inspectCompatResponse{ | ||
| InspectResponse: resp, | ||
| legacyConfig: legacyConfigFields["current"], | ||
| } |
There was a problem hiding this comment.
FWIW; I started to look if adding a constructor for this would be cleaner, and possibly it could be, but rewriting took me down the rabbit hole, so I'm gonna leave that for a future refactor; possibly combining all the version-related changes below 🤔
| // Legacy fields for current API versions (v1.50 and up). These fields | ||
| // did not have an "omitempty" and were always included in the response, | ||
| // even if not set; see https://github.com/moby/moby/issues/50134 | ||
| "current": { | ||
| "Cmd": nil, | ||
| "Entrypoint": nil, | ||
| "Env": nil, | ||
| "Labels": nil, | ||
| "OnBuild": nil, | ||
| "User": "", | ||
| "Volumes": nil, | ||
| "WorkingDir": "", | ||
| }, |
There was a problem hiding this comment.
I think we can avoid this one and just marshal the base InspectResponse instead?
There was a problem hiding this comment.
These were the ones that had an omit empty, so were not included #50134
So I think using the base would then result in them disappearing again.
But for api v1.51 we can consider doing so, but we'd have to deprecate that behavior first (can do that in a separate PR)
There was a problem hiding this comment.
Right...
I think what confused me is that the caller is responsible for passing the correct value of legacyConfigFields.
Could we just pass the API version the response should be adapted to?
So
imageInspect := &inspectCompatResponse{
InspectResponse: resp,
}would mean the "current"/"latest" and
imageInspect := &inspectCompatResponse{
InspectResponse: resp,
version: "1.49"
}would mean the 1.49 compatible response..
Although that's probably what you meant in https://github.com/moby/moby/pull/50135/files#r2126024062?
There was a problem hiding this comment.
Btw, I think it would be slightly easier to understand if we declared a copy-pasted, unexported versions of the "legacy" Config struct and just explicitly filled and marshalled them inside inspectCompatResponse.MarshalJSON.
While more verbose, I think it would be much clearer and easier to understand.
Just a thought though, not a blocker
There was a problem hiding this comment.
Could we just pass the API version the response should be adapted to?
Yeah, that's something I was considering, and mostly had written, then threw it away thinking to KISS for now. I can still have a look at that though.
I think it would be slightly easier to understand if we declared a copy-pasted, unexported versions of the "legacy" Config struct
Ah, gotcha. Hm.. yes, possibly could work. My goal at least was to avoid having to depend on legacy types or fields if that meant we'd have to continue maintaining the fields (as "deprecated") purely for the old responses (but with empty / zero values).
Having a (non-exported) copy of such structs purely for that purpose is indeed slightly better; what I slightly tried to avoid though was to (in some cases) that potentially requiring to also use legacy indirect types, such as the nat.Port mapping in the code we had;
moby/api/server/router/image/image_routes.go
Lines 604 to 609 in 9663b36
I recall we used to have these "versioned" legacy types, which I guess is a bit of that approach; some of those started to become quite involved though;
https://github.com/moby/moby/blob/v19.03.0/api/types/versions/v1p19/types.go
https://github.com/moby/moby/blob/v19.03.0/api/types/versions/v1p20/types.go
https://github.com/moby/moby/blob/v19.03.0/daemon/inspect.go#L22-L30
commit 4dc961d removed deprecated fields from the image inspect response for API v1.50 and up. As part of that change, it changed the type used for the Config field to use the docker image spect structs, which embeds the OCI image spec structs. While the OCI image spect struct contains the same fields as we used before, those fields also have "omitempty" set, which means they are now omitted when empty. We should probably consider deprecating that behavior in the API, and call out that these fields are omitted if not set, but in the meantime, we can add them back with their default (zero) value. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
4ff4fa2 to
f85394d
Compare
|
Squashed the commits, but happy to make additional changes related to #50135 (comment) |
commit 4dc961d removed deprecated fields from the image inspect response for API v1.50 and up. As part of that change, it changed the type used for the Config field to use the docker image spect structs, which embeds the OCI image spec structs.
While the OCI image spect struct contains the same fields as we used before, those fields also have "omitempty" set, which means they are now omitted when empty.
We should probably consider deprecating that behavior in the API, and call out that these fields are omitted if not set, but in the meantime, we can add them back with their default (zero) value.
- What I did
- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)