From fd3414c054e8a3442b59e818f6ccc29847e2f73a Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 12 Jan 2023 16:12:28 +0000 Subject: [PATCH] fix: refactor periodic update checks Added a global check to avoid spamming checking for updates on every invocation, which was slow. Remove the specific upgradce/version caching, as is wasn't needed give the above. Altogether, much simpler and future proof --- opensafely/__init__.py | 52 +++++++++++++++++++-------- opensafely/upgrade.py | 23 +++--------- tests/test_init.py | 18 ++++++++++ tests/test_upgrade.py | 80 +++++++++++------------------------------- 4 files changed, 79 insertions(+), 94 deletions(-) create mode 100644 tests/test_init.py diff --git a/opensafely/__init__.py b/opensafely/__init__.py index 5287e1d..234bbb4 100644 --- a/opensafely/__init__.py +++ b/opensafely/__init__.py @@ -1,5 +1,7 @@ import argparse import sys +import tempfile +from datetime import datetime, timedelta from pathlib import Path # ensure pkg_resources can find the package metadata we have included, as the @@ -27,6 +29,22 @@ __version__ = Path(__file__).parent.joinpath("VERSION").read_text().strip() +VERSION_FILE = Path(tempfile.gettempdir()) / "opensafely-version-check" + + +def should_version_check(): + if not VERSION_FILE.exists(): + return True + + four_hours_ago = datetime.utcnow() - timedelta(hours=4) + + return VERSION_FILE.stat().st_mtime < four_hours_ago.timestamp() + + +def update_version_check(): + VERSION_FILE.touch() + + def main(): parser = argparse.ArgumentParser() @@ -62,21 +80,25 @@ def add_subcommand(cmd, module): add_subcommand("info", info) add_subcommand("exec", execute) - # 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 + # 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() args = parser.parse_args() kwargs = vars(args) diff --git a/opensafely/upgrade.py b/opensafely/upgrade.py index 3817859..a52488e 100644 --- a/opensafely/upgrade.py +++ b/opensafely/upgrade.py @@ -2,8 +2,6 @@ import shutil import subprocess import sys -import tempfile -from datetime import datetime, timedelta from pathlib import Path import opensafely @@ -12,8 +10,6 @@ DESCRIPTION = "Upgrade the opensafely cli tool." -CACHE_FILE = Path(tempfile.gettempdir()) / "opensafely-latest-version" - def add_arguments(parser): parser.add_argument( @@ -26,7 +22,7 @@ def add_arguments(parser): def main(version): if version == "latest": - version = get_latest_version(force=True) + version = get_latest_version() if not need_to_update(version): print(f"opensafely is already at version {version}") @@ -62,20 +58,9 @@ def main(version): sys.exit(exc) -def get_latest_version(force=False): - latest = None - two_hours_ago = datetime.utcnow() - timedelta(hours=2) - - if CACHE_FILE.exists(): - if CACHE_FILE.stat().st_mtime > two_hours_ago.timestamp(): - latest = CACHE_FILE.read_text().strip() - - if force or latest is None: - resp = requests.get("https://pypi.org/pypi/opensafely/json").json() - latest = resp["info"]["version"] - CACHE_FILE.write_text(latest) - - return latest +def get_latest_version(): + resp = requests.get("https://pypi.org/pypi/opensafely/json").json() + return resp["info"]["version"] def comparable(version_string): diff --git a/tests/test_init.py b/tests/test_init.py new file mode 100644 index 0000000..5a1724b --- /dev/null +++ b/tests/test_init.py @@ -0,0 +1,18 @@ +import os +from datetime import datetime, timedelta + +import opensafely + + +def test_should_version_check(): + + opensafely.VERSION_FILE.unlink(missing_ok=True) + + assert opensafely.should_version_check() is True + opensafely.update_version_check() + assert opensafely.should_version_check() is False + + timestamp = (datetime.utcnow() - timedelta(hours=5)).timestamp() + os.utime(opensafely.VERSION_FILE, (timestamp, timestamp)) + + assert opensafely.should_version_check() is True diff --git a/tests/test_upgrade.py b/tests/test_upgrade.py index 569bcbe..29b78cd 100644 --- a/tests/test_upgrade.py +++ b/tests/test_upgrade.py @@ -1,7 +1,5 @@ import argparse -import os import sys -from datetime import datetime, timedelta import pytest from requests_mock import mocker @@ -17,12 +15,6 @@ mocker._original_send = requests.Session.send -@pytest.fixture(autouse=True) -def clean_cache_file(): - if upgrade.CACHE_FILE.exists(): - upgrade.CACHE_FILE.unlink() - - @pytest.fixture def set_current_version(monkeypatch): def set(value): @@ -32,11 +24,19 @@ def set(value): yield set -def test_main_latest_upgrade(requests_mock, run, set_current_version): - requests_mock.get( - "https://pypi.org/pypi/opensafely/json", - json={"info": {"version": "1.1.0"}}, - ) +@pytest.fixture +def set_pypi_version(requests_mock): + def set(version): + 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): + set_pypi_version("1.1.0") set_current_version("v1.0.0") run.expect( [sys.executable, "-m", "pip", "install", "--upgrade", "opensafely==1.1.0"] @@ -44,61 +44,21 @@ def test_main_latest_upgrade(requests_mock, run, set_current_version): upgrade.main("latest") -def test_main_latest_no_upgrade(requests_mock, run, set_current_version, capsys): - requests_mock.get( - "https://pypi.org/pypi/opensafely/json", - json={"info": {"version": "1.0.0"}}, - ) +def test_main_latest_no_upgrade(set_pypi_version, run, set_current_version, capsys): + set_pypi_version("1.0.0") set_current_version("v1.0.0") upgrade.main("latest") out, err = capsys.readouterr() assert out.strip() == "opensafely is already at version 1.0.0" -def test_main_specifi_no_upgrade(run, set_current_version, capsys): +def test_main_specific_no_upgrade(run, set_current_version, capsys): set_current_version("v1.1.0") upgrade.main("1.1.0") out, err = capsys.readouterr() assert out.strip() == "opensafely is already at version 1.1.0" -def test_get_latest_version_no_cache(requests_mock): - requests_mock.get( - "https://pypi.org/pypi/opensafely/json", - json={"info": {"version": "1.1.0"}}, - ) - - assert not upgrade.CACHE_FILE.exists() - assert upgrade.get_latest_version() == "1.1.0" - assert upgrade.CACHE_FILE.read_text() == "1.1.0" - - -def test_get_latest_version_with_cache(requests_mock): - requests_mock.get( - "https://pypi.org/pypi/opensafely/json", - json={"info": {"version": "1.1.0"}}, - ) - upgrade.CACHE_FILE.write_text("1.0.0") - assert upgrade.get_latest_version() == "1.0.0" - assert upgrade.get_latest_version(force=True) == "1.1.0" - - -def test_get_latest_version_cache_expired(requests_mock): - requests_mock.get( - "https://pypi.org/pypi/opensafely/json", - json={"info": {"version": "1.1.0"}}, - ) - upgrade.CACHE_FILE.write_text("1.0.0") - # set mtime to 1 day ago - a_day_ago = (datetime.utcnow() - timedelta(days=1)).timestamp() - os.utime(upgrade.CACHE_FILE, (a_day_ago, a_day_ago)) - - # ignores cache and uses the newer latest version from requests - # check cache updated - assert upgrade.get_latest_version() == "1.1.0" - assert upgrade.CACHE_FILE.read_text() == "1.1.0" - - def test_need_to_update(set_current_version): set_current_version("v1.0.0") assert upgrade.need_to_update("1.1.0") @@ -112,8 +72,8 @@ 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, capsys): - upgrade.CACHE_FILE.write_text("1.1.0") +def test_check_version_needs_updating(set_current_version, set_pypi_version, capsys): + set_pypi_version("1.1.0") set_current_version("v1.0.0") assert upgrade.check_version() _, err = capsys.readouterr() @@ -124,8 +84,8 @@ def test_check_version_needs_updating(set_current_version, capsys): ] -def test_check_version_not_need_updating(set_current_version, capsys): - upgrade.CACHE_FILE.write_text("1.0.0") +def test_check_version_not_need_updating(set_current_version, set_pypi_version, capsys): + set_pypi_version("1.0.0") set_current_version("v1.0.0") assert not upgrade.check_version() out, _ = capsys.readouterr()