-
Notifications
You must be signed in to change notification settings - Fork 18.9k
api: info: deprecate "Commit.Expected" fields #48478
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
Conversation
The `Commit` type was introduced in 2790ac6, to assist triaging issues that were reported with an incorrect version of runc or containerd. At the time, both `runc` and `containerd` were not yet stable, and had to be built from a specific commit to guarantee compatibility. We encountered various situations where unexpected (and incompatible) versions of those binaries were packaged, resulting in hard to trace bug-reports. For those situations, a "expected" version was set at compile time, to indicate if the version installed was different from the expected version; docker info ... runc version: a592beb5bc4c4092b1b1bac971afed27687340c5 (expected: 69663f0bd4b60df09991c08812a60108003fa340) Both `runc` and `containerd` are stable now, and docker 19.03 and up set the expected version to the actual version since c65f0bd and 23.0 did the same for the `init` binary b585c64, to prevent the CLI from reporting "unexpected version". In short; the `Expected` fields no longer serves a real purpose. In future, we can even consider deprecating the `ContainerdCommit`, `RuncCommit` and `InitCommit` fields on the `/info` response (as we also include this information as part of the components returned in `/version`), but those can still be useful currently for situations where a user only provides `docker info` output. This patch starts with deprecating the `Expected` field. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
48867ca to
ff191c5
Compare
laurazard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| description: | | ||
| Commit ID of external tool expected by dockerd as set at build time. | ||
| **Deprecated**: This field is deprecated and will be omitted in a API v1.49. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in a response, right? So to actually omit it in 1.49+ we'll have to do something like omitempty on it (or a split/separate struct)? Would we then just make sure the code setting the value is conditional? (Making sure we're planning for how that's going to work/happen, and maybe even how we'll remember to implement that since the implementation isn't included in this PR that I can see 😅 perhaps it could be, since we do already know which API version it should be removed in?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this PR only did the deprecation part, but I had follow-up changes stashed to handle the omitempty and to make it conditional; that still required an API version bump, so didn't push those changes yet, and I kept the omitempty out, in case there would be contention around "omit" or "keep, but empty".
Just pushed a WIP branch with those stashed changes if you're interested in the approach I had in mind 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to post the link;
relates to:
The
Committype was introduced in 2790ac6, to assist triaging issues that were reported with an incorrect version of runc or containerd. At the time, bothruncandcontainerdwere not yet stable, and had to be built from a specific commit to guarantee compatibility.We encountered various situations where unexpected (and incompatible) versions of those binaries were packaged, resulting in hard to trace bug-reports. For those situations, a "expected" version was set at compile time, to indicate if the version installed was different from the expected version;
Both
runcandcontainerdare stable now, and docker 19.03 and up set the expected version to the actual version since c65f0bd and 23.0 did the same for theinitbinary b585c64, to prevent the CLI from reporting "unexpected version".In short; the
Expectedfields no longer serves a real purpose.In future, we can even consider deprecating the
ContainerdCommit,RuncCommitandInitCommitfields on the/inforesponse (as we also include this information as part of the components returned in/version), but those can still be useful currently for situations where a user only providesdocker infooutput.This patch starts with deprecating the
Expectedfield.- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)