From e409aec180757012c36204ac48509559a5e7c261 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 21 Aug 2024 12:15:07 +0100 Subject: [PATCH 1/6] Move some fixtures into conftest.py for broader use --- tests/conftest.py | 30 ++++++++++++++++++++++++++++++ tests/test_upgrade.py | 29 ----------------------------- 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index ddea745..c0cfbd4 100644 --- a/tests/conftest.py +++ b/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 diff --git a/tests/test_upgrade.py b/tests/test_upgrade.py index 8b48de2..6dc6cbf 100644 --- a/tests/test_upgrade.py +++ b/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): From fe358704b8efa0cc0ba483f7fa83a6e40cc20312 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 21 Aug 2024 12:17:38 +0100 Subject: [PATCH 2/6] Move code for printing warnings into __init__ It was awkward that the pull and upgrade modules had print statements anyway. Move the logic into __init__, and corresponding tests. This allows use to use the check_version functions from other modules, and only print the warning when we intend to. --- opensafely/__init__.py | 64 +++++++++++++++++++++++++++++------------- opensafely/pull.py | 6 ---- opensafely/upgrade.py | 12 +++----- tests/test_init.py | 37 ++++++++++++++++++++++++ tests/test_pull.py | 15 ++-------- tests/test_upgrade.py | 12 ++------ 6 files changed, 89 insertions(+), 57 deletions(-) diff --git a/opensafely/__init__.py b/opensafely/__init__.py index 7dc5e23..76abbbf 100644 --- a/opensafely/__init__.py +++ b/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) diff --git a/opensafely/pull.py b/opensafely/pull.py index d3c28e4..cc81a44 100644 --- a/opensafely/pull.py +++ b/opensafely/pull.py @@ -222,10 +222,4 @@ def check_version(): 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 diff --git a/opensafely/upgrade.py b/opensafely/upgrade.py index a52488e..9bc6df2 100644 --- a/opensafely/upgrade.py +++ b/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 diff --git a/tests/test_init.py b/tests/test_init.py index e5122a1..cdcf2b6 100644 --- a/tests/test_init.py +++ b/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", + "", + ] diff --git a/tests/test_pull.py b/tests/test_pull.py index 3447766..4c0c505 100644 --- a/tests/test_pull.py +++ b/tests/test_pull.py @@ -166,23 +166,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 +183,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(): diff --git a/tests/test_upgrade.py b/tests/test_upgrade.py index 6dc6cbf..f533d66 100644 --- a/tests/test_upgrade.py +++ b/tests/test_upgrade.py @@ -43,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( From fa4ff89bcfd1b122029941281a001a61e51544cd Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 21 Aug 2024 12:22:38 +0100 Subject: [PATCH 3/6] Ignore dangling images when finding local images Sometimes, users have dangling imates, with no tag. Before this change, this *broke* `opensafely pull` as we didn't handle them. Since dangling images should will be removed when we run the prune after we've pulled pull, they don't really matter. Ignoring them here means opensafely pull will still work, as only check images that have tags. It also means that the dict that get_local_images() returns is clean and had no noise, which is useful in the next commit --- opensafely/pull.py | 2 ++ tests/test_pull.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/opensafely/pull.py b/opensafely/pull.py index cc81a44..6229867 100644 --- a/opensafely/pull.py +++ b/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}}", ], diff --git a/tests/test_pull.py b/tests/test_pull.py index 4c0c505..2436f1e 100644 --- a/tests/test_pull.py +++ b/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}}", ], From 9e8b0e0ebaaa7c21c80e5e0947dc76706db1d4a9 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 21 Aug 2024 12:25:37 +0100 Subject: [PATCH 4/6] Have `opensafely info` also list installed docker images Including their versions and whether they are out of date This is useful when asking users to provide information when debugging problems. --- opensafely/info.py | 13 +++++++++++++ opensafely/pull.py | 5 +++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/opensafely/info.py b/opensafely/info.py index c29b140..53ce0cc 100644 --- a/opensafely/info.py +++ b/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}") diff --git a/opensafely/pull.py b/opensafely/pull.py index 6229867..fe99411 100644 --- a/opensafely/pull.py +++ b/opensafely/pull.py @@ -214,9 +214,10 @@ 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(":") From 2d5054c8232436958bcc10048bcb9ab8b8e97521 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 21 Aug 2024 12:27:02 +0100 Subject: [PATCH 5/6] drive by formatting change --- DEVELOPERS.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/DEVELOPERS.md b/DEVELOPERS.md index 256e686..9cb9769 100644 --- a/DEVELOPERS.md +++ b/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. From e9aa76daf5bf66caa46ce4820e221c94aa5bec56 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Wed, 21 Aug 2024 14:20:34 +0100 Subject: [PATCH 6/6] fix: ignore dangling images to unstick opensafely pull