save: Remove platform from config descriptor#47661
Conversation
tianon
left a comment
tianon
left a comment
There was a problem hiding this comment.
Agreed, this is unexpected -- I've thought about putting it here myself before, but I think I'd personally classify this as a case of "undefined behavior" in the spec, since platform should technically only be set on descriptors in an index (https://github.com/opencontainers/image-spec/blob/v1.1.0/descriptor.md#:~:text=Descriptors%20pointing%20to%20application/vnd.oci.image.manifest.v1%2Bjson%20SHOULD%20include%20the%20extended%20field%20platform).
IMO a decent alternative would be to set data on this config blob, but maybe only if it's below a certain size (so we ensure it isn't the thing that pushes us over the 4MiB recommended/enforced-by-at-least-Docker-Hub manifest size limit).
This was brought up by bmitch that its not expected to have a platform object in the config descriptor. Also checked with tianon who agreed, its not _wrong_ but is unexpected and doesn't neccessarily make sense to have it there. Also, while technically incorrect, ECR is throwing an error when it sees this. Signed-off-by: Brian Goff <[email protected]>
cpuguy83
force-pushed
the
oci_tar_no_platform
branch
from
f084bc4 to
9160b9f
Compare
|
Repushed to fix typo in commit title. |
cpuguy83
changed the title
vvoland
left a comment
vvoland
left a comment
There was a problem hiding this comment.
LGTM; do we want to highlight this in the release notes?
|
@cpuguy83 can you write a short blurb for the release-notes? 🤗 |
|
I took a stab at a changelog entry, PTAL |
|
Thanks @tianon |
vvoland
added
process/cherry-picked
and removed
process/cherry-pick
process/cherry-pick/25.0
process/cherry-pick/26.0
labels
4 participants
This was brought up by bmitch that its not expected to have a platform object in the config descriptor.
Also checked with tianon who agreed, its not wrong but is unexpected and doesn't neccessarily make sense to have it there.
Also, while technically incorrect, ECR is throwing an error when it sees this.