-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Stop using legacy cache import/export in client #2982
Stop using legacy cache import/export in client #2982
Conversation
This also fixes the bug where registry cache options with a missing ref would be silently discarded by the server-side component that turns the legacy settings into the new settings. Signed-off-by: Jonny Stoten <jonny.stoten@docker.com>
Signed-off-by: Jonny Stoten <jonny.stoten@docker.com>
Signed-off-by: Jonny Stoten <jonny.stoten@docker.com>
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.
Could you also update the proto type to add indication that these fields have been removed. Eg. rename from "Deprecated" suffix to "Removed" or add comment.
Not "removed" yet (until vNextNext) ? |
@AkihiroSuda These were already deprecated in v0.4 |
Yes, deprecated, but not removed and still in use by buildctl as of v0.10.x. |
Isn't it only used when buildctl v0.10 is talking with v0.3 daemon. I don't think we care about that case. |
Doesn't seem so 😥 https://github.com/moby/buildkit/blob/v0.10.3/client/solve.go#L467-L475 |
Yes, my thinking is that we remove that stuff from the client first (this PR) and then after the next release we can remove the daemon-side usage and rename to "Removed" suffix. |
Fixes #2974.
I've kept the server-side code for translating the legacy request for now so that this will continue to work with existing clients. We should be able to delete this once enough clients have been updated, perhaps in the following release?
I've also added some simple validation in the client for the
ref
attr on registry cache options, and some extra context around errors from theResolveCacheExporterFunc
s.