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

Support remote images with registry host:port #939

Merged
merged 4 commits into from
Aug 5, 2019

Conversation

atemate
Copy link
Contributor

@atemate atemate commented Aug 1, 2019

The newest version of neuro-API 19.7.26 broke platform-monitoring: neuro-inc/platform-monitoring#70

The problem was that if registry_url contains both host and port, still _ImageNameParser._registry will contain only host part. Thus, parser _container_from_api, used to parse the response for Jobs.run, will try to parse an image like localhost:5000/user/ubuntu:latest as a local image not as remote (because it does not start with localhost/ but starts with localhost:5000/).

Note: one test is still xfailed due to #938
(it fails with a wrong message)

@asvetlov
Copy link
Contributor

asvetlov commented Aug 1, 2019

Hmm.
localhost:5000/user/ubuntu:latest is an URL with localhost scheme.
I'm sure that it is not what you want.
Correct URL should be either https://localhost:5000/user/ubuntu:latest or //localhost:5000/user/ubuntu:latest if you want to omit scheme.

Otherwise the parser is ambiguous.

@atemate atemate requested a review from asvetlov August 1, 2019 14:22
@atemate
Copy link
Contributor Author

atemate commented Aug 1, 2019

thank you @asvetlov for your idea with //localhost:5000/user/ubuntu:latest! simplified the code.

tests/api/test_images.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #939 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #939     +/-   ##
=========================================
+ Coverage   91.92%   92.03%   +0.1%     
=========================================
  Files          37       37             
  Lines        4209     4216      +7     
  Branches      634      636      +2     
=========================================
+ Hits         3869     3880     +11     
+ Misses        230      227      -3     
+ Partials      110      109      -1
Impacted Files Coverage Δ
neuromation/api/parsing_utils.py 100% <100%> (ø) ⬆️
neuromation/cli/main.py 69.8% <0%> (+1.98%) ⬆️

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 a278ba4...115d009. Read the comment docs.

if not url.scheme:
parts = url.path.split("/")
url = url.build(
url = URL.build(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

class method, IMO it's better to use class name here

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Please add changenote

@atemate
Copy link
Contributor Author

atemate commented Aug 5, 2019

I will add the changelog message to the second PR that resolves this issue: #940

@atemate atemate merged commit 2db4567 into master Aug 5, 2019
@atemate atemate deleted the ay/fix-image-parser-registry-with-port branch August 5, 2019 14:57
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.

4 participants