Merge pull request #178 from opensafely-core/stop-being-slow

fix: refactor periodic update checks

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)

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):

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

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,73 +24,41 @@ 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"]
     )
     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()