-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add support for platform parameter in image push #3325
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,6 +3,7 @@ | |||||
|
||||||
from .. import auth, errors, utils | ||||||
from ..constants import DEFAULT_DATA_CHUNK_SIZE | ||||||
from ..types.image import Platform | ||||||
|
||||||
log = logging.getLogger(__name__) | ||||||
|
||||||
|
@@ -434,7 +435,7 @@ def pull(self, repository, tag=None, stream=False, auth_config=None, | |||||
return self._result(response) | ||||||
|
||||||
def push(self, repository, tag=None, stream=False, auth_config=None, | ||||||
decode=False): | ||||||
decode=False, platform=None): | ||||||
""" | ||||||
Push an image or a repository to the registry. Similar to the ``docker | ||||||
push`` command. | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be an instance of the new |
||||||
|
||||||
Returns: | ||||||
(generator or str): The output from the server. | ||||||
|
@@ -488,6 +490,13 @@ def push(self, repository, tag=None, stream=False, auth_config=None, | |||||
log.debug('Sending supplied auth config') | ||||||
headers['X-Registry-Auth'] = auth.encode_header(auth_config) | ||||||
|
||||||
if platform is not None: | ||||||
if utils.version_lt(self._version, '1.46'): | ||||||
raise errors.InvalidVersion( | ||||||
'platform was only introduced in API version 1.46' | ||||||
) | ||||||
params['platform'] = Platform | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why
Suggested change
|
||||||
|
||||||
response = self._post_json( | ||||||
u, None, headers=headers, stream=stream, params=params | ||||||
) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
from .base import DictType | ||
|
||
|
||
class Platform(DictType): | ||
def __init__(self, **kwargs): | ||
architecture = kwargs.get('architecture', kwargs.get('Architecture')) | ||
os = kwargs.get('os', kwargs.get('OS')) | ||
|
||
if architecture is None and os is None: | ||
raise ValueError("At least one of 'architecture' or 'os' must be provided") | ||
|
||
|
||
super().__init__({ | ||
'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')) | ||
Comment on lines
+14
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be lowercase, also I don't think there's the need to support the Titlecased fields. |
||
}) | ||
|
||
@property | ||
def architecture(self): | ||
return self['Architecture'] | ||
|
||
@property | ||
def os(self): | ||
return self['OS'] | ||
|
||
@architecture.setter | ||
def architecture(self, value): | ||
self['Architecture'] = value | ||
|
||
@os.setter | ||
def os(self, value): | ||
self['OS'] = value |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vvoland can you take a look at this?