-
Notifications
You must be signed in to change notification settings - Fork 527
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
crane rebase and mutate.Rebase set empty-valued platform fields #751
Comments
This seems reasonable to me. @jonjohnsonjr wdyt? |
I'm surprised that we're dropping any fields -- we should be preserving everything and just updating the things that need to change. Agree this is a bug. |
Thanks for the quick response! Would a PR from me be useful? I realize there might be some shared behavior between |
Definitely. Happy to merge a quick fix. Ultimately I'd like to understand why we're dropping these fields, I don't think it's intentional. Doing what
I think this is an uncontroversial change and wouldn't worry about backward compatibility. |
* Quick fix for missing os/architecture properties, required by config spec (https://github.com/opencontainers/image-spec/blob/master/config.md#properties) * Also include unspec'd os.version property which is required for Windows images to run on Windows containers. * This implementation takes these three properties from newBase as it most accurately defines the new rebased image's format. If the properties were taken from original, it would prevent reuse of ABI-compatible top layers from differing old and new base images (i.e. data-only top layers, Windows minor version upgrades). Fixes [google#751] Signed-off-by: Micah Young <ymicah@vmware.com>
* Quick fix for missing `os` and `architecture` config properties, required by config spec [1] * Also include unspec'd `os.version` property which is required for Windows images to run on Windows containers [2] This implementation takes these three properties from newBase, which most accurately defines the newly rebased image's os/architecture. If the properties were taken from original, it would prevent reuse of de-facto ABI-compatible top layers with differing old and new base images (i.e. data-only top layers rebased from amd64 to arm, Windows os.version upgrades). This does not address additional, optional config properties that are currently being omitted during rebase such as `author` and `created`. Fixes [google#751] Signed-off-by: Micah Young <ymicah@vmware.com> [1]: https://github.com/opencontainers/image-spec/blob/master/config.md#properties [2]: https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility?tabs=windows-server-2004%2Cwindows-10-2004#revision-number-patching Signed-off-by: Micah Young <ymicah@vmware.com>
* Quick fix for missing `os` and `architecture` config properties, required by config spec [1] * Also include unspec'd `os.version` property which is required for Windows images to run on Windows containers [2] This implementation takes these three properties from newBase, which most accurately defines the newly rebased image's os/architecture. If the properties were taken from original, it would prevent reuse of de-facto ABI-compatible top layers with differing old and new base images (i.e. data-only top layers rebased from amd64 to arm, Windows os.version upgrades). This does not address additional, optional config properties that are currently being omitted during rebase such as `author` and `created`. Fixes [google#751] [1]: https://github.com/opencontainers/image-spec/blob/master/config.md#properties [2]: https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility#matching-container-host-version-with-container-image-versions Signed-off-by: Micah Young <ymicah@vmware.com>
* Quick fix for missing `os` and `architecture` config properties, required by config spec [1] * Also include unspec'd `os.version` property which is required for Windows images to run on Windows containers [2] This implementation takes these three properties from newBase, which most accurately defines the newly rebased image's os/architecture. If the properties were taken from original, it would prevent reuse of de-facto ABI-compatible top layers with differing old and new base images (i.e. data-only top layers rebased from amd64 to arm, Windows os.version upgrades). This does not address additional, optional config properties that are currently being omitted during rebase such as `author` and `created`. Fixes [#751] [1]: https://github.com/opencontainers/image-spec/blob/master/config.md#properties [2]: https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility#matching-container-host-version-with-container-image-versions Signed-off-by: Micah Young <ymicah@vmware.com>
After rebasing, the
rebased
image hasplatform
fields with empty values ("os":""
,"architecture":""
,"os.version":""
, etc.) though the input images contained these fields with valid values.Docker and k8s Linux containers allow pulling and running these platform-less images by adding their own platform but Windows containers fail to pull or run (which is how I noticed this issue).
It appears this is mostly implemented already in
mutate.Time
though some optional fields are missed (os.features
,variant
,features
fromplatform
struct).I feel the expected behavior would be to copy all platform fields with values (spec'd under
platform
) from thenew_base
image to therebased
image.I'll add a PR assuming there's no objection to the proposed behavior.
Repro:
The text was updated successfully, but these errors were encountered: