Skip to content
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

fix: dont overwrite docker image names #1365

Merged
merged 1 commit into from
Nov 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions jina/docker/hubapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import os
import pkgutil
from pkgutil import iter_modules
from typing import Dict, Sequence, Any, Optional
from typing import Dict, Sequence, Any, Optional, List
from urllib.error import HTTPError
from urllib.parse import urlencode
from urllib.request import Request, urlopen
Expand Down Expand Up @@ -68,7 +68,7 @@ def _list_local(logger) -> Optional[Dict[str, Any]]:


def _list(logger, image_name: str = None, image_kind: str = None,
image_type: str = None, image_keywords: Sequence = ()) -> Optional[Dict[str, Any]]:
image_type: str = None, image_keywords: Sequence = ()) -> Optional[List[Dict[str, Any]]]:
""" Use Hub api to get the list of filtered images

:param logger: logger to use
Expand Down
40 changes: 34 additions & 6 deletions jina/docker/hubio.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
import webbrowser
from typing import Dict, Any

from docker import DockerClient
from docker.errors import ImageNotFound, APIError

from jina import __version__ as jina_version
from .checker import *
from .helper import credentials_file
Expand All @@ -22,7 +25,7 @@
from ..importer import ImportExtensions
from ..logging import JinaLogger
from ..logging.profile import TimeContext
from ..parser import set_pod_parser
from ..parser import set_pod_parser, set_hub_list_parser
from ..peapods import Pod

if False:
Expand Down Expand Up @@ -57,7 +60,7 @@ def _load_docker_client(self):
import docker
from docker import APIClient

self._client = docker.from_env()
self._client: DockerClient = docker.from_env()

# low-level client
self._raw_client = APIClient(base_url='unix://var/run/docker.sock')
Expand Down Expand Up @@ -171,6 +174,16 @@ def push(self, name: str = None, readme_path: str = None, build_result: Dict = N
name = name or self.args.name

try:
# check if image exists
# fail if it does
if self._image_version_exists(
build_result['manifest_info']['name'],
build_result['manifest_info']['version'],
jina_version
):
raise Exception(f'Image with name {name} already exists. Will NOT overwrite.')
else:
self.logger.debug(f'Image with name {name} does not exist. Pushing now...')
self._push_docker_hub(name, readme_path)

if not build_result:
Expand Down Expand Up @@ -253,8 +266,9 @@ def _check_docker_image(self, name: str) -> None:
if f'{_label_prefix}{r}' not in image.labels.keys():
self.logger.warning(f'{r} is missing in your docker image labels, you may want to check it')
try:
image.labels['jina_version'] = jina_version
if name != safe_url_name(
f'{self.args.repository}/' + '{type}.{kind}.{name}:{version}'.format(
f'{self.args.repository}/' + '{type}.{kind}.{name}:{version}-{jina_version}'.format(
**{k.replace(_label_prefix, ''): v for k, v in image.labels.items()})):
raise ValueError(f'image {name} does not match with label info in the image')
except KeyError:
Expand Down Expand Up @@ -489,9 +503,10 @@ def _check_completeness(self) -> Dict:

self.manifest = self._read_manifest(self.manifest_path)
self.dockerfile_path_revised = self._get_revised_dockerfile(self.dockerfile_path, self.manifest)
self.manifest['jina_version'] = jina_version
self.tag = safe_url_name(f'{self.args.repository}/' + '{type}.{kind}.{name}:{version}-{jina_version}'.format(**self.manifest))
self.canonical_name = safe_url_name(f'{self.args.repository}/' + '{type}.{kind}.{name}'.format(**self.manifest))
tag_name = safe_url_name(
f'{self.args.repository}/' + f'{self.manifest["type"]}.{self.manifest["kind"]}.{self.manifest["name"]}:{self.manifest["version"]}-{jina_version}')
self.tag = tag_name
self.canonical_name = tag_name
return completeness

def _read_manifest(self, path: str, validate: bool = True) -> Dict:
Expand Down Expand Up @@ -584,3 +599,16 @@ def _expand_fn(v):
# alias of "new" in cli
create = new
init = new

def _image_version_exists(self, name, module_version, req_jina_version):
manifests = _list(self.logger, name)
# check if matching module version and jina version exists
if manifests:
matching = [
m for m in manifests
if m['version'] == module_version
and 'jina_version' in m.keys()
and m['jina_version'] == req_jina_version
]
return len(matching) > 0
return True
33 changes: 33 additions & 0 deletions tests/unit/docker/test_hub_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,39 @@ def test_hub_build_push():
assert manifests[0]['name'] == summary['manifest_info']['name']


def test_hub_build_push_push_again():
args = set_hub_build_parser().parse_args([os.path.join(cur_dir, 'hub-mwu'), '--push', '--host-info'])
summary = HubIO(args).build()

with open(os.path.join(cur_dir, 'hub-mwu', 'manifest.yml')) as fp:
manifest = yaml.load(fp)

assert summary['is_build_success']
assert manifest['version'] == summary['version']
assert manifest['description'] == summary['manifest_info']['description']
assert manifest['author'] == summary['manifest_info']['author']
assert manifest['kind'] == summary['manifest_info']['kind']
assert manifest['type'] == summary['manifest_info']['type']
assert manifest['vendor'] == summary['manifest_info']['vendor']
assert manifest['keywords'] == summary['manifest_info']['keywords']

args = set_hub_list_parser().parse_args([
'--name', summary['manifest_info']['name'],
'--keywords', summary['manifest_info']['keywords'][0],
'--type', summary['manifest_info']['type']
])
response = HubIO(args).list()
manifests = response

assert len(manifests) >= 1
assert manifests[0]['name'] == summary['manifest_info']['name']

# try and push same version again should fail
args = set_hub_build_parser().parse_args([os.path.join(cur_dir, 'hub-mwu'), '--push', '--host-info'])
summary = HubIO(args).build()
print(summary['is_build_success'])
Comment on lines +96 to +98
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An assertion for the exception is expected here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to restore the PR and edit it? Or should I just create a new one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a new one better



def test_hub_build_failures():
for j in ['bad-dockerfile', 'bad-pythonfile', 'missing-dockerfile', 'missing-manifest']:
args = set_hub_build_parser().parse_args(
Expand Down