vendor: buildkit v0.11.4-0.20230228113103-218e934edfba#45095
vendor: buildkit v0.11.4-0.20230228113103-218e934edfba#45095thaJeztah merged 2 commits intomoby:masterfrom
Conversation
|
Linter failure: We're practically importing this one for one const: const ImageConfigField = "moby.buildkit.buildinfo.v1"I don't see any other package in buildkit which defines this one, so I guess we should just inline it? Related PR: moby/buildkit#3614 \cc @crazy-max |
|
@vvoland Yes you can inline it in the meantime. This feature will be removed for next 0.12, see moby/buildkit#3582. See also https://github.com/moby/buildkit/blob/v0.11/docs/deprecated.md#build-information |
| // const ( | ||
| // emptyGZLayer = digest.Digest("sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1") | ||
| // ) |
There was a problem hiding this comment.
Unrelated to this PR, but was this "no longer used" or "not yet used"?
There was a problem hiding this comment.
Looks like no longer used. Last reference to this was removed in: 0728fb2
So I'd say it's okay to remove?
| // const ( | ||
| // emptyGZLayer = digest.Digest("sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1") | ||
| // ) | ||
| const imageConfigField = "moby.buildkit.buildinfo.v1" |
There was a problem hiding this comment.
So, slightly wondering;
- As this is the master branch; does it already use the replacement? If so, perhaps we should (can we?) remove the functionality as a whole?
- If we cannot remove it now, the alternative would be to add a
//nolint:...(with a comment about the deprecation)
Advantage of the //nolint: would be that we deliberately make the build break once we would update BuildKit to v0.12, that way we don't accidentally forget about removing it once it's gone (whereas having a local const means we could still have (dead)code.
There was a problem hiding this comment.
Oh it's the master branch. Then yeah better to remove it entirely. related commit: fda0226
There was a problem hiding this comment.
Thanks! Yes, so this is master (23.1.0 "dev"), which will be the first release with BuildKit 0.11, but hasn't been released yet, so perhaps it's fine to deprecate/remove ahead of BuildKit.
So my suggestion would be;
- Open a PR to mark the feature deprecated (from the "Docker Engine" perspective) in https://github.com/docker/cli/blob/master/docs/deprecated.md. We can probably mark it as "deprecated in v23.0.0", and "to be removed in v23.1.0" (as it was already deprecated in BuildKit, but we didn't mention it)
- Remove it from master
- Vendor buildkit v0.11 "latest"
There was a problem hiding this comment.
But I could use feedback on that suggestion, as I'm not very familiar with the feature (and who/how it's used) 😅
|
Flaky test (just commenting to allow us finding it back) |
d888689 to
af5dbad
Compare
af5dbad to
439389c
Compare
|
This one can be rebased |
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
439389c to
3e4c4df
Compare
|
Rebased, are fine with sneaking the remove commented dead const in here, or should I make it a separate PR? |
|
I see you did in a separate commit, and it's a trivial changes, so seems fine to keep it here |
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
(we're temporarily ahead of the last BuildKit release, but there likely will be a v0.11.4 with some bug fixes before we release)
|
@rumpl @neersighted ptal 🤗 |
|
let me bring this one in to unblock #44079 |
changes: moby/buildkit@v0.11.3...218e934
- 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)