Check the image is outdated and pop warning to pull the latest #193

unkcpz · 2023-10-03T21:17:17Z

fixes #188

unkcpz · 2023-10-03T21:20:46Z

It is quite hard to test the feature added here since ~~1) don't know how to test the image_is_latest function which rely on the real image~~ ~~2) don't know how to test the warning is properly raised in the start stage which are currently in the life cycle test.~~

I test it manually and works as I expected.

EDIT: the added util function is tested by checking with a light weight image.
EDIT: see the last commit where I tried to test this feature. But has to import the util as a module otherwise the function can not be overridden. Not sure this is proper.

codecov · 2023-10-03T21:22:56Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (1eedb9d) 86.32% compared to head (5d3cf30) 86.11%.

❗ Current head 5d3cf30 differs from pull request most recent head 42f0867. Consider uploading reports for the commit 42f0867 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
- Coverage   86.32%   86.11%   -0.21%     
==========================================
  Files           9        9              
  Lines         914      929      +15     
==========================================
+ Hits          789      800      +11     
- Misses        125      129       +4

Flag	Coverage Δ
py-3.10	`86.00% <73.33%> (-0.21%)`	⬇️
py-3.11	`86.00% <73.33%> (-0.21%)`	⬇️
py-3.8	`85.96% <73.33%> (-0.21%)`	⬇️
py-3.9	`86.06% <73.33%> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
aiidalab_launch/__main__.py	`79.81% <100.00%> (+0.18%)`	⬆️
aiidalab_launch/util.py	`82.85% <66.66%> (-1.52%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielhollas

Great, thanks for improving this and adding a test!

Just a couple of small suggestions.

danielhollas · 2023-10-04T16:22:24Z

tests/test_util.py

+def test_image_is_latest(docker_client):
+    """Test that the latest version is identified correctly."""
+    # download the alpine image for testing
+    image_name = "alpine:latest"


Let's use something more lightweight, perhaps the hello-world image from Docker?

Also, it is not clear to me, is this image then automatically deleted in enable_docker_pull fixture?

more lightweight, perhaps the hello-world image from Docker?

👍

is this image then automatically deleted in enable_docker_pull fixture?

No, I don't think so. This fixture is to guarantee the test will be skipped if it contains docker pull.

What a surprise, the hello-world is bigger than alpine. 😄

https://hub.docker.com/_/hello-world/tags

https://hub.docker.com/_/alpine/tags

Huh. But that seems to be only true for for windows (which does not exist for Alpine), for linux it is only a couple of kB, i.e. an order of magnitute smaller. So I would still prefer to switch to hello-world image, and mark this test as skipped if ran on windows (I don't think we support windows anyway, but just to ensure we do not end up downloading 100Mb in case anybody tried it).

Also since these tests rely on the network, I would prefer if they were marked as "slow".

danielhollas · 2023-10-04T16:23:46Z

tests/test_util.py

+    image_name = "alpine:latest"
+    docker_client.images.pull(image_name)
+
+    # check that the image is identified as latest


It would be good to also test the converse, i.e. pull some outdated version of the same image and check that it is identified as not latest. That should be possible right? (should be a separate test to keep it clean)

I think it can, by pulling the old image and retag it. See the new commit.

danielhollas · 2023-10-04T16:30:55Z

aiidalab_launch/__main__.py

+                "Warning! Local image is outdated, please run with --pull to update.",
+                fg="yellow",
+            )
+
    if instance.image is None:
        raise click.ClickException(
            f"Unable to find image '{profile.image}'. "


Can you please delete the line below. We now automatically try to download the image when we do not find it locally, so this advice is not helpful.

You are right, I wasn't aware of it in #184, this conditional never be hit.

danielhollas

Thanks @unkcpz, I have a couple more comments. My biggest concern here is to not accidentaly delete all images on the machine. 😰

danielhollas · 2023-10-06T14:10:37Z

aiidalab_launch/util.py

+    # there is a new image with the same tag, the id will be different.
+    # We can not use image id, see https://windsock.io/explaining-docker-image-ids/
+    local_digest = local_image.attrs.get("RepoDigests")[0].split("@")[-1]
+    remote_digest = remote_image.attrs.get("Descriptor", {}).get("digest")


What if the remote_digest ends up being None here? We should probably return true in that case, since that is likely a problem with the Dockerhub and we should not pull. (or it could indicate that the API changed)

danielhollas · 2023-10-06T14:11:32Z

aiidalab_launch/util.py

+    # There is no need to check creation date of the image, since the once
+    # there is a new image with the same tag, the id will be different.
+    # We can not use image id, see https://windsock.io/explaining-docker-image-ids/
+    local_digest = local_image.attrs.get("RepoDigests")[0].split("@")[-1]


I am a little unhappy about the split()[-1] here since that does not seem very robust. But if there is not a better way of getting the digest than I guess it is fine.

danielhollas · 2023-10-06T14:13:56Z

tests/conftest.py

@@ -57,6 +57,19 @@ def docker_client():
        pytest.skip("docker not available")


+@pytest.fixture(scope="function")
+def remove_created_images(docker_client):


I am a little nervous here, how does this ensure that we do not wipe out all the images on the machine? Is that ensured by the docker_client fixture?

Even if it supposedly is, I would be much more comfortable if we filtered the image names somehow (e.g. the image name needs to contain test or some other unique string.

Thinking about this more, this approach is definitely not safe since, since other processes can create docker images in the meantime while the test is running. The logic here would delete these images.

Instead we could perhaps hard-code the name of the test image (`hello-world"), which should always be safe to delete.

danielhollas · 2023-10-06T14:16:59Z

tests/test_cli.py

+        result: Result = runner.invoke(
+            cli.cli, ["start", "--no-browser", "--no-pull", "--wait=300"]
+        )
+        assert "Warning!" in result.output.strip()


Could you get this assert be a more specific than just "Warning"? That is too generic.
Also please also check the exit code and that the container is up and has already been running, as done above?

danielhollas · 2023-10-06T14:18:01Z

tests/test_cli.py

@@ -260,6 +260,20 @@ def assert_status_down():
        assert result.exit_code == 0
        assert_status_up()

+        # test the warning message of image not the latest is not raised
+        assert "Warning!" not in result.output.strip()


Hmm, you could maybe have this assert earlier after the container is first started. Then you could move the monkeypatch above as well and avoid starting up the container for the third time.

danielhollas · 2023-10-06T14:27:46Z

tests/test_util.py

+def test_image_is_latest(docker_client):
+    """Test that the latest version is identified correctly."""
+    # download the alpine image for testing
+    image_name = "alpine:latest"


Huh. But that seems to be only true for for windows (which does not exist for Alpine), for linux it is only a couple of kB, i.e. an order of magnitute smaller. So I would still prefer to switch to hello-world image, and mark this test as skipped if ran on windows (I don't think we support windows anyway, but just to ensure we do not end up downloading 100Mb in case anybody tried it).

Also since these tests rely on the network, I would prefer if they were marked as "slow".

unkcpz requested a review from danielhollas October 3, 2023 21:20

unkcpz force-pushed the fix/188/warn-when-new-image-avail branch from b884cd9 to e658d11 Compare October 3, 2023 21:34

danielhollas requested changes Oct 4, 2023

View reviewed changes

unkcpz added 3 commits October 6, 2023 10:26

Check the image is outdated and pop warning to pull the latest

3d7e022

monkeypatch and test the warning pop

6dbb0c4

f-d

42f0867

unkcpz force-pushed the fix/188/warn-when-new-image-avail branch from 1fabd9f to 42f0867 Compare October 6, 2023 08:27

unkcpz requested a review from danielhollas October 6, 2023 08:29

danielhollas requested changes Oct 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check the image is outdated and pop warning to pull the latest #193

Check the image is outdated and pop warning to pull the latest #193

unkcpz commented Oct 3, 2023

unkcpz commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

danielhollas left a comment

danielhollas Oct 4, 2023

unkcpz Oct 6, 2023 •

edited

Loading

unkcpz Oct 6, 2023

danielhollas Oct 6, 2023

danielhollas Oct 4, 2023

unkcpz Oct 6, 2023 •

edited

Loading

danielhollas Oct 4, 2023

unkcpz Oct 6, 2023

danielhollas left a comment

danielhollas Oct 6, 2023

danielhollas Oct 6, 2023

danielhollas Oct 6, 2023

danielhollas Feb 4, 2024

danielhollas Oct 6, 2023

danielhollas Oct 6, 2023

danielhollas Oct 6, 2023

Check the image is outdated and pop warning to pull the latest #193

Are you sure you want to change the base?

Check the image is outdated and pop warning to pull the latest #193

Conversation

unkcpz commented Oct 3, 2023

unkcpz commented Oct 3, 2023 • edited Loading

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz Oct 6, 2023 • 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

unkcpz Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas left a 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

unkcpz commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

unkcpz Oct 6, 2023 •

edited

Loading

unkcpz Oct 6, 2023 •

edited

Loading