Add support for platform parameter in image push #3325
Conversation
Signed-off-by: Khushiyant <khushiyant2002@gmail.com>
Khushiyant
force-pushed
the
image-push-platform
branch
from
3dd9b4f to
2356a17
Compare
|
Any chance someone can look at this? This causing a bug for me. What happens without this is tags can’t be pushed to registries. Only the sha. Say I copy a docker image from one registry to another. You want to copy a specific platform, then it won’t push the tag without specifying the platform. |
| @@ -448,6 +448,7 @@ def push(self, repository, tag=None, stream=False, auth_config=None, | |||
| ``username`` and ``password`` keys to be valid. | |||
| decode (bool): Decode the JSON data from the server into dicts. | |||
| Only applies with ``stream=True`` | |||
| platform (str): JSON-encoded OCI platform to select the platform-variant to push. If not provided, all available variants will attempt to be pushed. | |||
There was a problem hiding this comment.
It's expected to be a JSON-encoded at the API level. But at the SDK level, it would probably be better if it was an instance of a nicer "Platform" class/struct.
There was a problem hiding this comment.
Whatever the case shouldn’t it match the pull? There shouldn’t be a string in one and class in another.
There was a problem hiding this comment.
No, these have different format. Pull accepts a plain platform string like linux/amd64, but this one is a full JSON representing an OCI platform.
But you're right, that it's inconsistent and we probably should fix it at some point.
There was a problem hiding this comment.
@vvoland can you take a look at this?
| @@ -448,6 +449,7 @@ def push(self, repository, tag=None, stream=False, auth_config=None, | |||
| ``username`` and ``password`` keys to be valid. | |||
| decode (bool): Decode the JSON data from the server into dicts. | |||
| Only applies with ``stream=True`` | |||
| platform (str): JSON-encoded OCI platform to select the platform-variant to push. If not provided, all available variants will attempt to be pushed. | |||
There was a problem hiding this comment.
This should be an instance of the new Platform
| raise errors.InvalidVersion( | ||
| 'platform was only introduced in API version 1.46' | ||
| ) | ||
| params['platform'] = Platform |
There was a problem hiding this comment.
Why Platform (class) instead of the platform (instance)`?
| params['platform'] = Platform | |
| params['platform'] = platform |
| 'Architecture': architecture, | ||
| 'OS': os, | ||
| 'OSVersion': kwargs.get('os_version', kwargs.get('OSVersion')), | ||
| 'OSFeatures': kwargs.get('os_features', kwargs.get('OSFeatures')), | ||
| 'Variant': kwargs.get('variant', kwargs.get('Variant')) |
There was a problem hiding this comment.
I think this could be lowercase, also I don't think there's the need to support the Titlecased fields.
Closes #3324
Adds the following:
platformparameter handling inimages.api.pushtests/unit/api_image_test.py::ImageTest::test_push_image_with_platform