api: remove / internalize LegacyDiskUsage#51437
Conversation
75998fc to
a62598d
Compare
These fields have been removed from the API specification, and the struct was only needed to produce legacy responses (server), or to unmarshal legacy responses in the client. As the API module only provides API definitions for the current API version, we should remove these legacy structs, and keep them internal to the daemon and client. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
a62598d to
ed42823
Compare
vvoland
left a comment
There was a problem hiding this comment.
Considering that legacy fields are still returned if the verbose is not true, shouldn't we leave this for v1.53?
|
That's the tricky bit; we won't be able to remove it anymore once we have a v1.x release. If I'm not mistaken we already removed the legacy fields from the API definition, so effectively they're un-documented fields that are no longer part of the API; Lines 10488 to 10506 in a0e4deb Given that both the API and Client are a new module, and the client (with this change) doesn't depend on the struct to be present, this allows us to phase out the legacy fields once API < v1.52 support is dropped. Perhaps we should even consider already omitting the fields in API v1.52; I THINK the only reason we currently still include it is because we didn't have time yet to look at |
| `verbose` query parameter is used. Starting with API v1.53, the legacy fields | ||
| will no longer be returned. |
There was a problem hiding this comment.
Should we tie removal of the legacy fields to a release of the daemon rather than an API version? (If we find an issue, we might bump the API version quite soon in a minor/patch release, but should probably carry this forward to a major release?)
There was a problem hiding this comment.
If needed, we could extend the grace-period of course. It's a bit of an odd-one-out overall; effectively, the fields have already been removed from the API, so API v1.52 should already omit them (and no longer defines them), so from that perspective, perhaps the API changelog should not document the fallback behavior at all (and it's mostly an implementation-specific fallback, where the dockerd daemon chose to provide a grace-period).
Not sure what the best place is to document that though; perhaps in deprecated.md (CLI repo)?
There was a problem hiding this comment.
Ok - when we do drop the fields, after Docker release notes, I guess the API changelog will be where people look to see why their stuff broke. So, SGTM.
api: remove / internalize LegacyDiskUsage
These fields have been removed from the API specification, and the struct
was only needed to produce legacy responses (server), or to unmarshal
legacy responses in the client.
As the API module only provides API definitions for the current API version,
we should remove these legacy structs, and keep them internal to the daemon
and client.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)