-
Notifications
You must be signed in to change notification settings - Fork 390
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
Gitlab registry support #1283
Gitlab registry support #1283
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 |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
from http.client import responses | ||
import json | ||
import string | ||
import re | ||
import time | ||
import escapism | ||
|
||
|
@@ -55,6 +56,30 @@ | |
LAUNCHES_INPROGRESS = Gauge('binderhub_inprogress_launches', 'Launches currently in progress') | ||
|
||
|
||
def _get_image_basename_and_tag(full_name): | ||
"""Get a supposed image name and tag without the registry part | ||
:param full_name: full image specification, e.g. "gitlab.com/user/project:tag" | ||
:return: tuple of image name and tag, e.g. ("user/project", "tag") | ||
""" | ||
# the tag is either after the last (and only) colon, or not given at all, | ||
# in which case "latest" is implied | ||
tag_splits = full_name.rsplit(':', 1) | ||
if len(tag_splits) == 2: | ||
image_name = tag_splits[0] | ||
tag = tag_splits[1] | ||
else: | ||
image_name = full_name | ||
tag = 'latest' | ||
|
||
if re.fullmatch('[a-z0-9]{4,40}/[a-z0-9._-]{2,255}', image_name): | ||
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. Could we not instead just check for the number of 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. Not so sure. What if we have self-host registries without any paths to specify user or project? The unit test shows an example: This distinction cannot be covered by just counting |
||
# if it looks like a Docker Hub image name, we're done | ||
return image_name, tag | ||
# if the image isn't implied to origin at Docker Hub, | ||
# the first part has to be a registry | ||
image_basename = '/'.join(image_name.split('/')[1:]) | ||
return image_basename, tag | ||
|
||
|
||
def _generate_build_name(build_slug, ref, prefix='', limit=63, ref_length=6): | ||
"""Generate a unique build name with a limited character length. | ||
|
||
|
@@ -310,7 +335,7 @@ async def get(self, provider_prefix, _unescaped_spec): | |
if self.settings['use_registry']: | ||
for _ in range(3): | ||
try: | ||
image_manifest = await self.registry.get_image_manifest(*'/'.join(image_name.split('/')[-2:]).split(':', 1)) | ||
image_manifest = await self.registry.get_image_manifest(*_get_image_basename_and_tag(image_name)) | ||
image_found = bool(image_manifest) | ||
break | ||
except HTTPClientError: | ||
|
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.
In the current implementation, I think the tag is left unspecified if it isn't present. Let's preserve that?
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.
I wouldn't mind. If i see correctly, the tag is passed to
registry.get_image_manifest()
where the following happens:"{}/v2/{}/manifests/{}".format(self.url, image, tag)
So if tag is an empty string, could this be a problem for the manifests API? Idk personally, as i am pretty much clueless about this.
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.
@rappertomate I think tag is just always required to be specified when pushing, and is never left empty. I wouldn't mind raising an exception if tag is empty or None, but we can tackle that in another PR. I mostly just wanted to make sure the behavior doesn't change.
I think once that's done I'll happily merge this.