Load and display image tags for `neuro images` #873

anayden · 2019-07-01T15:23:57Z

Addresses #852

atemate · 2019-07-01T15:40:58Z

neuromation/cli/image.py

@@ -114,7 +114,8 @@ def image() -> None:

    images = await root.client.images.ls()
    for image in images:
-        click.echo(image)
+        for tag in images[image]:
+            click.echo(f"{image} - {tag}")


how about we output images with tags in the format that we can re-use for other commands? I believe, it's image://user/ubuntu:tag

atemate · 2019-07-01T15:42:52Z

neuromation/api/images.py

+            result: Dict[URL, List[str]] = {}
+            for repo in ret["repositories"]:
+                if repo.startswith("image://"):
+                    name = str(repo)[8:]  # len("image://") = 8


let's create a constant for it, prefix = "image://"; name = str(repo)[len(prefix):]

atemate · 2019-07-01T15:44:07Z

neuromation/api/images.py

+                    url = URL(f"image://{repo}")
+                result[url] = []
+                async with self._registry.request(
+                    "GET", URL(f"{name}/tags/list")


if this is not yet implemented in platform-registry, let's comment this out for now or wrap with try-catch-ignore

This has been merged already

asvetlov · 2019-07-03T14:31:31Z

Your implementation has O(N*M) complexity which is not desirable.

Please add a new client.images.tags(image) method for /v2/*/tags/list and keep /v2/_catalog untouched.

serhiy-storchaka · 2019-07-03T14:37:27Z

@anayden, you can use "Closes #xxx" or "Fixes #xxx" to close automatically the corresponded issue.

codecov · 2019-07-05T14:54:48Z

Codecov Report

Merging #873 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #873      +/-   ##
==========================================
- Coverage   91.37%   91.24%   -0.13%     
==========================================
  Files          37       37              
  Lines        3617     3610       -7     
  Branches      527      520       -7     
==========================================
- Hits         3305     3294      -11     
- Misses        213      218       +5     
+ Partials       99       98       -1

Impacted Files	Coverage Δ
neuromation/api/images.py	`95.12% <100%> (+0.43%)`	⬆️
neuromation/cli/image.py	`100% <100%> (ø)`	⬆️
neuromation/cli/storage.py	`78.07% <0%> (-6.83%)`	⬇️

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 539d4c6...e62b6fe. Read the comment docs.

codecov · 2019-07-05T14:54:48Z

Codecov Report

Merging #873 into master will decrease coverage by 1.37%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #873      +/-   ##
==========================================
- Coverage   91.37%   89.99%   -1.38%     
==========================================
  Files          37       38       +1     
  Lines        3617     3608       -9     
  Branches      527      516      -11     
==========================================
- Hits         3305     3247      -58     
- Misses        213      263      +50     
+ Partials       99       98       -1

Impacted Files	Coverage Δ
neuromation/api/images.py	`93.33% <100%> (-1.36%)`	⬇️
neuromation/cli/image.py	`100% <100%> (ø)`	⬆️
neuromation/api/utils.py	`80% <0%> (-16.67%)`	⬇️
neuromation/cli/storage.py	`77.27% <0%> (-7.62%)`	⬇️
neuromation/cli/utils.py	`88.1% <0%> (-3.35%)`	⬇️
neuromation/cli/main.py	`59.7% <0%> (-2%)`	⬇️
neuromation/api/jobs.py	`94.26% <0%> (-1.86%)`	⬇️
neuromation/cli/formatters/jobs.py	`95.65% <0%> (-1.74%)`	⬇️
neuromation/api/abc.py	`91.66% <0%> (-0.65%)`	⬇️
... and 5 more

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 539d4c6...3658b0c. Read the comment docs.

serhiy-storchaka · 2019-07-05T16:48:23Z

neuromation/api/images.py

+        name = str(image)[len(prefix) :]
+        async with self._registry.request("GET", URL(f"{name}/tags/list")) as resp:
+            ret = await resp.json()
+            return [tag for tag in ret.get("tags", [])]


Why not just return ret.get("tags", [])?

ah, of course! that's an artefact from refactoring

serhiy-storchaka · 2019-07-05T16:50:20Z

neuromation/cli/image.py

+
+    Examples:
+
+    neuro image tags image://myfriend/alpine:shared


So the argument can contain a tag?

No, it can't. Fixed the example

serhiy-storchaka · 2019-07-05T17:21:07Z

neuromation/cli/image.py

@@ -117,6 +118,27 @@ def image() -> None:
        click.echo(image)


+@command()
+@click.argument("image_name")


It is not an image name. May be rename the argument to simply image?

serhiy-storchaka · 2019-07-05T17:22:25Z

neuromation/api/images.py

+
+    async def tags(self, image: URL) -> List[str]:
+        prefix = "image://"
+        name = str(image)[len(prefix) :]


What if image does not start with "image://"?

Good point. Added a check and a test for that behavior

serhiy-storchaka · 2019-07-08T11:23:08Z

neuromation/cli/image.py

+@command()
+@click.argument("image_name")
+@async_cmd()
+async def tags(root: Root, image_name: str) -> None:


The PR's title says about neuro images. Should we add an alias neuro images or correct the title and the original issue?

Since we work with a single image here, I believe the command should be neuro image, not neuro images. I think the issue should be rephrased
cc @dalazx

serhiy-storchaka · 2019-07-08T11:25:22Z

neuromation/cli/image.py

+    """
+    List tags for image in platform registry.
+
+    Image name must be URL with image:// scheme.


Is it needed to add support for relative URIs like image:myimage?

please use ImageNameParser to parse image: str to image: DockerImage

I think, CLI methods for all image operations must use the same image URI parser -- this is the only way not to confuse the users

There is a difference. Other operations accept image URI with tag, but this operation should only accept image URI without tag.

atemate · 2019-07-08T11:40:32Z

neuromation/api/images.py

                result.append(url)
            return result
+
+    async def tags(self, image: URL) -> List[str]:


I think, we should accept image: str similarly to API's method async def run(...), which accepts Container with image: str

I don't care right now.
It may be changed in the following PR.
I'm working on images API stabilization and have no final decision yet.

I've added DockerImage support. This will work even with images with tags, but I'd keep this as undocumented and not officially supported feature :)

asvetlov

Let's merge when green

anayden requested a review from asvetlov July 1, 2019 15:24

astaff added CLIents platform labels Jul 1, 2019

atemate reviewed Jul 1, 2019

View reviewed changes

anayden force-pushed the an/load_tags branch from f165e30 to 28ded8c Compare July 2, 2019 12:41

anayden added 8 commits July 5, 2019 16:20

Temp

d17bb9f

Print tags

a28fccc

Minor

7f68ba7

Add e2e tests

130080f

Address code review comments

bbe51c5

Fix other e2e tests

160cd9d

Add new tags action

e9427c9

Add neuro image tags command

3658b0c

anayden force-pushed the an/load_tags branch from 28ded8c to 3658b0c Compare July 5, 2019 14:54

serhiy-storchaka reviewed Jul 5, 2019

View reviewed changes

Address code review comments

0450a35

serhiy-storchaka reviewed Jul 8, 2019

View reviewed changes

Rename image_name to image

1e9dfcd

atemate reviewed Jul 8, 2019

View reviewed changes

asvetlov approved these changes Jul 8, 2019

View reviewed changes

anayden added 2 commits July 8, 2019 18:05

Use image parser for image name in neuro image tags

934fbb4

remove unused import

e62b6fe

anayden merged commit e700202 into master Jul 8, 2019

anayden deleted the an/load_tags branch July 8, 2019 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load and display image tags for `neuro images` #873

Load and display image tags for `neuro images` #873

anayden commented Jul 1, 2019

atemate Jul 1, 2019

atemate Jul 1, 2019

atemate Jul 1, 2019

anayden Jul 2, 2019

asvetlov commented Jul 3, 2019

serhiy-storchaka commented Jul 3, 2019

codecov bot commented Jul 5, 2019 •

edited

Loading

codecov bot commented Jul 5, 2019

serhiy-storchaka Jul 5, 2019

anayden Jul 8, 2019

serhiy-storchaka Jul 5, 2019

anayden Jul 8, 2019 •

edited

Loading

serhiy-storchaka Jul 5, 2019

serhiy-storchaka Jul 5, 2019

anayden Jul 8, 2019

serhiy-storchaka Jul 8, 2019

anayden Jul 8, 2019

serhiy-storchaka Jul 8, 2019

atemate Jul 8, 2019

atemate Jul 8, 2019

serhiy-storchaka Jul 8, 2019

atemate Jul 8, 2019

asvetlov Jul 8, 2019

anayden Jul 8, 2019

asvetlov left a comment

Load and display image tags for neuro images #873

Load and display image tags for neuro images #873

Conversation

anayden commented Jul 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Jul 3, 2019

serhiy-storchaka commented Jul 3, 2019

codecov bot commented Jul 5, 2019 • edited Loading

Codecov Report

codecov bot commented Jul 5, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anayden Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

Load and display image tags for `neuro images` #873

Load and display image tags for `neuro images` #873

codecov bot commented Jul 5, 2019 •

edited

Loading

anayden Jul 8, 2019 •

edited

Loading