Merge pull request #264 from opensafely-core/handle-dangling-images

fix: Ignore dangling images in opensafely pull

DEVELOPERS.md

@@ -3,7 +3,7 @@
 ## Vendoring
 
 To minimise the possibility for installation issues this package vendors
-all its dependencies under the [opensafely._vendor](./opensafely/_vendor)
+all its dependencies under the [`opensafely._vendor`](./opensafely/_vendor)
 namespace using the [vendoring](https://pypi.org/project/vendoring/) tool.
 
 This brings its own complexities (particularly around the `requests`
@@ -23,9 +23,9 @@ To update the vendored version of job-runner:
    ```
 
 2. Run the update script. It will default to latest tag, but you can pass
-   a specific job-runner tag.  
+   a specific job-runner tag.
    ```
-   ./scripts/update.sh [version_tag] 
+   ./scripts/update.sh [version_tag]
    ```
 
 3. Commit the results
@@ -46,7 +46,7 @@ known as "software engineering".
 
 ## Releases
 
-New versions are tagged, and the PyPI package built and published, automatically 
-via GitHub actions on merges to `main`.  Note that the version will be bumped 
+New versions are tagged, and the PyPI package built and published, automatically
+via GitHub actions on merges to `main`.  Note that the version will be bumped
 according to information parsed from the commits using semantic-release conventions (e.g. `fix:`, `feat:`).  If no semantic commit is found since the last tag, a new version
 will not be released.

opensafely/__init__.py

@@ -47,10 +47,53 @@ def should_version_check():
     return VERSION_FILE.stat().st_mtime < four_hours_ago.timestamp()
 
 
+# separated out to be able to used in test setup
 def update_version_check():
     VERSION_FILE.touch()
 
 
+def warn_if_updates_needed(argv):
+    # we only check for new versions periodically
+    if not should_version_check():
+        return
+
+    # we version check before parse_args is called so that if a user is
+    # following recent documentation but has an old opensafely installed,
+    # there's some hint as to why their invocation is failing before being told
+    # by argparse
+    if len(argv) == 1 or argv[1] != "upgrade":
+        try:
+            latest = upgrade.check_version()
+            if latest:
+                print(
+                    f"Warning: there is a newer version of opensafely available ({latest})"
+                    " - please upgrade by running:\n"
+                    "    opensafely upgrade\n",
+                    file=sys.stderr,
+                )
+                # if we're out of date, don't warn the user about out of date images as well
+                return
+        except Exception:
+            pass
+
+    # check for out of date images unless we are updating images.
+    if len(argv) == 1 or argv[1] != "pull":
+        try:
+            need_update = pull.check_version()
+            if need_update:
+                print(
+                    f"Warning: the OpenSAFELY docker images for {', '.join(need_update)} actions are out of date"
+                    " - please update by running:\n"
+                    "    opensafely pull\n",
+                    file=sys.stderr,
+                )
+
+        except Exception:
+            pass
+
+    update_version_check()
+
+
 def main():
     parser = argparse.ArgumentParser()
 
@@ -94,26 +137,7 @@ def add_subcommand(cmd, module):
     add_subcommand("exec", execute)
     add_subcommand("clean", clean)
 
-    # we only check for new versions periodically
-    if should_version_check():
-        # we version check before parse_args is called so that if a user is
-        # following recent documentation but has an old opensafely installed,
-        # there's some hint as to why their invocation is failing before being told
-        # by argparse.
-        if len(sys.argv) == 1 or sys.argv[1] != "upgrade":
-            try:
-                upgrade.check_version()
-            except Exception:
-                pass
-
-        if len(sys.argv) == 1 or sys.argv[1] != "pull":
-            try:
-                pull.check_version()
-            except Exception:
-                pass
-
-        update_version_check()
-
+    warn_if_updates_needed(sys.argv)
     args = parser.parse_args()
     kwargs = vars(args)
 

opensafely/info.py

@@ -3,6 +3,7 @@
 import sys
 
 import opensafely
+from opensafely import pull
 
 
 DESCRIPTION = "Print information about the opensafely tool and available resources"
@@ -21,6 +22,7 @@ def add_arguments(parser):
 
 
 def main():
+    print("System information:")
     try:
         ps = subprocess.run(
             ["docker", "info", "-f", "{{json .}}"], capture_output=True, text=True
@@ -38,3 +40,14 @@ def main():
         )
     except Exception:
         sys.exit("Error retreiving docker information")
+
+    print("OpenSAFELY Docker image versions:")
+    try:
+        local_images = pull.get_local_images()
+        updates = pull.check_version(local_images)
+    except Exception:
+        sys.exit("Error retreiving image information")
+    else:
+        for image, sha in sorted(local_images.items()):
+            update = "(needs update)" if image in updates else "(latest version)"
+            print(f" - {image:24}: {sha[7:15]} {update}")

opensafely/pull.py

@@ -126,6 +126,8 @@ def get_local_images():
             "ghcr.io/opensafely-core/*",  # this excludes dev builds
             "--filter",
             "label=org.opensafely.action",
+            "--filter",
+            "dangling=false",  # these will be pruned
             "--no-trunc",
             "--format={{.Repository}}:{{.Tag}}={{.ID}}",
         ],
@@ -212,20 +214,15 @@ def get_auth_token(header):
     return auth_response.json()["token"]
 
 
-def check_version():
+def check_version(local_images=None):
     need_update = []
-    local_images = get_local_images()
+    if local_images is None:
+        local_images = get_local_images()
 
     for image, local_sha in local_images.items():
         name, _, tag = image.partition(":")
         latest_sha = get_remote_sha(f"{REGISTRY}/{name}", tag)
         if latest_sha != local_sha:
             need_update.append(image)
 
-    if need_update:
-        print(
-            f"Warning: the OpenSAFELY docker images for {', '.join(need_update)} actions are out of date - please update by running:\n"
-            "    opensafely pull\n",
-            file=sys.stderr,
-        )
     return need_update

opensafely/upgrade.py

@@ -80,11 +80,7 @@ def need_to_update(latest):
 
 def check_version():
     latest = get_latest_version()
-    update = need_to_update(latest)
-    if update:
-        print(
-            f"Warning: there is a newer version of opensafely available ({latest}) - please upgrade by running:\n"
-            "    opensafely upgrade\n",
-            file=sys.stderr,
-        )
-    return update
+    if need_to_update(latest):
+        return latest
+    else:
+        return False

tests/conftest.py

@@ -14,10 +14,20 @@
 pkg_resources.working_set.add_entry(f"{opensafely_module_dir}/_vendor")
 
 import pytest  # noqa: E402
+from requests_mock import mocker  # noqa: E402
 
+import opensafely  # noqa: E402
 from opensafely import utils  # noqa: E402
+from opensafely._vendor import requests  # noqa: E402
 
 
+# Because we're using a vendored version of requests we need to monkeypatch the
+# requests_mock library so it references our vendored library instead
+#
+mocker.requests = requests
+mocker._original_send = requests.Session.send
+
+# save reference to actual run function
 _actual_run = subprocess.run
 
 
@@ -173,3 +183,23 @@ def run_main(module, cli_args):
     argv = shlex.split(cli_args)
     args = parser.parse_args(argv)
     return module.main(**vars(args))
+
+
+@pytest.fixture
+def set_current_version(monkeypatch):
+    def set(value):  # noqa: A001
+        assert value[0] == "v", "Current __version__ must start with v"
+        monkeypatch.setattr(opensafely, "__version__", value)
+
+    yield set
+
+
+@pytest.fixture
+def set_pypi_version(requests_mock):
+    def set(version):  # noqa: A001
+        requests_mock.get(
+            "https://pypi.org/pypi/opensafely/json",
+            json={"info": {"version": version}},
+        )
+
+    return set

tests/test_init.py

@@ -2,6 +2,7 @@
 from datetime import datetime, timedelta
 
 import opensafely
+from tests.test_pull import expect_local_images
 
 
 def test_should_version_check():
@@ -15,3 +16,39 @@ def test_should_version_check():
     os.utime(opensafely.VERSION_FILE, (timestamp, timestamp))
 
     assert opensafely.should_version_check() is True
+
+
+def test_warn_if_updates_needed_package_outdated(
+    capsys, monkeypatch, tmp_path, set_current_version, set_pypi_version
+):
+
+    set_pypi_version("1.1.0")
+    set_current_version("v1.0.0")
+    monkeypatch.setattr(opensafely, "VERSION_FILE", tmp_path / "timestamp")
+    opensafely.warn_if_updates_needed(["opensafely"])
+
+    out, err = capsys.readouterr()
+    assert out == ""
+    assert err.splitlines() == [
+        "Warning: there is a newer version of opensafely available (1.1.0) - please upgrade by running:",
+        "    opensafely upgrade",
+        "",
+    ]
+
+
+def test_warn_if_updates_needed_images_outdated(capsys, monkeypatch, tmp_path, run):
+    monkeypatch.setattr(opensafely, "VERSION_FILE", tmp_path / "timestamp")
+    expect_local_images(
+        run,
+        stdout="ghcr.io/opensafely-core/python:latest=sha256:oldsha",
+    )
+
+    opensafely.warn_if_updates_needed(["opensafely"])
+
+    out, err = capsys.readouterr()
+    assert out == ""
+    assert err.splitlines() == [
+        "Warning: the OpenSAFELY docker images for python:latest actions are out of date - please update by running:",
+        "    opensafely pull",
+        "",
+    ]

tests/test_pull.py

@@ -21,6 +21,8 @@ def expect_local_images(run, stdout="", **kwargs):
             "ghcr.io/opensafely-core/*",
             "--filter",
             "label=org.opensafely.action",
+            "--filter",
+            "dangling=false",
             "--no-trunc",
             "--format={{.Repository}}:{{.Tag}}={{.ID}}",
         ],
@@ -166,23 +168,15 @@ def test_remove_deprecated_images(run):
     pull.remove_deprecated_images(local_images)
 
 
-def test_check_version_out_of_date(run, capsys):
+def test_check_version_out_of_date(run):
     expect_local_images(
         run,
         stdout="ghcr.io/opensafely-core/python:latest=sha256:oldsha",
     )
-
     assert len(pull.check_version()) == 1
-    out, err = capsys.readouterr()
-    assert out == ""
-    assert err.splitlines() == [
-        "Warning: the OpenSAFELY docker images for python:latest actions are out of date - please update by running:",
-        "    opensafely pull",
-        "",
-    ]
 
 
-def test_check_version_up_to_date(run, capsys):
+def test_check_version_up_to_date(run):
     current_sha = pull.get_remote_sha("ghcr.io/opensafely-core/python", "latest")
     pull.token = None
     expect_local_images(
@@ -191,9 +185,6 @@ def test_check_version_up_to_date(run, capsys):
     )
 
     assert len(pull.check_version()) == 0
-    out, err = capsys.readouterr()
-    assert err == ""
-    assert out.splitlines() == []
 
 
 def test_get_actions_from_project_yaml_no_actions():

tests/test_upgrade.py

@@ -2,37 +2,8 @@
 import sys
 
 import pytest
-from requests_mock import mocker
 
-import opensafely
 from opensafely import upgrade
-from opensafely._vendor import requests
-
-
-# Because we're using a vendored version of requests we need to monkeypatch the
-# requests_mock library so it references our vendored library instead
-mocker.requests = requests
-mocker._original_send = requests.Session.send
-
-
-@pytest.fixture
-def set_current_version(monkeypatch):
-    def set(value):  # noqa: A001
-        assert value[0] == "v", "Current __version__ must start with v"
-        monkeypatch.setattr(opensafely, "__version__", value)
-
-    yield set
-
-
-@pytest.fixture
-def set_pypi_version(requests_mock):
-    def set(version):  # noqa: A001
-        requests_mock.get(
-            "https://pypi.org/pypi/opensafely/json",
-            json={"info": {"version": version}},
-        )
-
-    return set
 
 
 def test_main_latest_upgrade(set_pypi_version, run, set_current_version):
@@ -72,24 +43,16 @@ def test_need_to_update(set_current_version):
     assert upgrade.need_to_update("1.11.0")
 
 
-def test_check_version_needs_updating(set_current_version, set_pypi_version, capsys):
+def test_check_version_needs_updating(set_current_version, set_pypi_version):
     set_pypi_version("1.1.0")
     set_current_version("v1.0.0")
     assert upgrade.check_version()
-    _, err = capsys.readouterr()
-    assert err.splitlines() == [
-        "Warning: there is a newer version of opensafely available (1.1.0) - please upgrade by running:",
-        "    opensafely upgrade",
-        "",
-    ]
 
 
-def test_check_version_not_need_updating(set_current_version, set_pypi_version, capsys):
+def test_check_version_not_need_updating(set_current_version, set_pypi_version):
     set_pypi_version("1.0.0")
     set_current_version("v1.0.0")
     assert not upgrade.check_version()
-    out, _ = capsys.readouterr()
-    assert out.strip() == ""
 
 
 @pytest.mark.parametrize(