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

Common way of parsing images: image:latest is forbidden #539

Merged
merged 30 commits into from
Feb 26, 2019

Conversation

atemate
Copy link
Contributor

@atemate atemate commented Feb 20, 2019

Sorry for such a big PR: we need to support all the cases of local images, remote images in docker-hub and remote images in neuro-registry:

$ neuro image push ubuntu
Using local image 'ubuntu:latest'
Using remote image 'image://artemyushkovskiy/ubuntu:latest'
...

$ neuro image push image://~/ubuntu
Invalid local image 'image://~/ubuntu': scheme 'image://' is not allowed for local images
...

$ neuro image pull image://~/ubuntu
Using remote image 'image://artemyushkovskiy/ubuntu:latest'
Using local image 'ubuntu:latest'
...

$ neuro image pull ubuntu
Invalid remote image 'ubuntu': scheme 'image://' is required
...

$ neuro submit ubuntu
Using image 'ubuntu:latest'
...

$ neuro submit image://~/ubuntu
Using image 'image://artemyushkovskiy/ubuntu:latest'
...

$ neuro submit image:ubuntu
Using image 'image://artemyushkovskiy/ubuntu:latest'
...

$ neuro submit image://chuck/ubuntu
Using image 'image://chuck/ubuntu:latest'
...

@atemate
Copy link
Contributor Author

atemate commented Feb 20, 2019

The corner case neuro submit image:v1 is resolved to image://~/v1:latest

@atemate
Copy link
Contributor Author

atemate commented Feb 20, 2019

maybe we need more tests on the commands (@command) itself (pull, push, submit), because e2e don't cover all the cases

@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #539 into master will increase coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #539     +/-   ##
=========================================
+ Coverage   91.46%   91.77%   +0.3%     
=========================================
  Files          39       40      +1     
  Lines        2941     3014     +73     
  Branches      384      391      +7     
=========================================
+ Hits         2690     2766     +76     
+ Misses        188      187      -1     
+ Partials       63       61      -2
Impacted Files Coverage Δ
python/neuromation/cli/model.py 87.3% <ø> (ø) ⬆️
python/neuromation/client/__init__.py 98.46% <100%> (+0.02%) ⬆️
python/neuromation/client/parsing_utils.py 100% <100%> (ø)
python/neuromation/cli/image.py 100% <100%> (+5.88%) ⬆️
python/neuromation/client/images.py 99.04% <100%> (-0.1%) ⬇️
python/neuromation/cli/job.py 96.96% <100%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df9ead5...d4c4972. Read the comment docs.

@atemate atemate force-pushed the ay/image-job-submit branch from 358d6a5 to 729351f Compare February 20, 2019 17:54
@atemate atemate changed the title Common way of parsing images Common way of parsing images: image:latest is image://~/latest:latest Feb 21, 2019
@atemate atemate changed the title Common way of parsing images: image:latest is image://~/latest:latest Common way of parsing images. image:latest is image://~/latest:latest Feb 21, 2019
@asvetlov
Copy link
Contributor

I prefer this variant.
image:abc is a URL with image scheme, and abc is for an image name.
BTW, docker hub has no image registered: https://hub.docker.com/_/image returns 404 Not Found

python/neuromation/client/parsing_utils.py Show resolved Hide resolved
python/neuromation/client/parsing_utils.py Outdated Show resolved Hide resolved
python/neuromation/cli/image.py Show resolved Hide resolved
python/neuromation/cli/image.py Show resolved Hide resolved
@atemate
Copy link
Contributor Author

atemate commented Feb 25, 2019

I prefer image:latest to be image://~/latest as well, because thus we follow our own pattern of using image: as scheme and not as name. Plus, indeed there's no such image latest in docker.io.

@asvetlov
Copy link
Contributor

We can forbid the image named latest for image: scheme explicitly to avoid confusion.

@atemate
Copy link
Contributor Author

atemate commented Feb 26, 2019

We can forbid the image named latest for image: scheme explicitly to avoid confusion.

Good idea! added this check:

$ neuro job submit image:latest
Invalid remote image 'image:latest': ambiguous value: valid as both local and remote image name

@atemate atemate changed the title Common way of parsing images. image:latest is image://~/latest:latest Common way of parsing images: image:latest is forbidden Feb 26, 2019
@dalazx
Copy link
Contributor

dalazx commented Feb 26, 2019

@ajuszkowski please resolve the conflict.

@atemate
Copy link
Contributor Author

atemate commented Feb 26, 2019

rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants