From e070589fb1d3dc947b15eabe20233a8afc949d99 Mon Sep 17 00:00:00 2001 From: Alec Scott Date: Wed, 19 Aug 2026 14:57:08 -0700 Subject: [PATCH 1/5] Remove repository config caching logic Signed-off-by: Alec Scott --- .github/workflows/requirements/unit-tests.txt | 1 - pyproject.toml | 1 - src/hubcast/web/github/routes.py | 28 +++--- src/hubcast/web/github/utils.py | 33 ++----- tests/test_github_routes.py | 32 ------- tests/test_repo_config.py | 88 ++++--------------- 6 files changed, 35 insertions(+), 148 deletions(-) diff --git a/.github/workflows/requirements/unit-tests.txt b/.github/workflows/requirements/unit-tests.txt index 002c5b36..cbb6e5e4 100644 --- a/.github/workflows/requirements/unit-tests.txt +++ b/.github/workflows/requirements/unit-tests.txt @@ -1,6 +1,5 @@ aiohttp==3.14.3 build==1.5.0 -cachetools==7.1.7 coverage==7.15.4 gidgethub==5.4.0 gidgetlab==2.1.2 diff --git a/pyproject.toml b/pyproject.toml index 7c23e580..d14b8344 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,7 +8,6 @@ version = "0.0.1" dependencies = [ "aiohttp", "aiojobs", - "cachetools", "gidgethub", "gidgetlab>=2.1.2", "pydantic", diff --git a/src/hubcast/web/github/routes.py b/src/hubcast/web/github/routes.py index d2122aae..92c0b2d7 100644 --- a/src/hubcast/web/github/routes.py +++ b/src/hubcast/web/github/routes.py @@ -285,15 +285,12 @@ async def sync_branch( log.info("Skipped branch sync - branch has open PR") return - # only refresh config when a default branch push touches .github/hubcast.yml default_branch = event.data["repository"]["default_branch"] is_default_branch = sync_ref == f"refs/heads/{default_branch}" changed_files = changed_files_from_push(event.data) config_changed = gh.repo_config_path in changed_files try: - repo_config = await get_repo_config( - gh, src_fullname, refresh=is_default_branch and config_changed - ) + repo_config = await get_repo_config(gh) except RepoConfigError as exc: # only report the default branch config's error when this push isn't trying to fix it if is_default_branch or not config_changed: @@ -312,13 +309,10 @@ async def sync_branch( head_commit = event.data.get("head_commit") commit_msg = head_commit["message"] if head_commit else "" - # only set/update webhook on default branch pushes when the push actually - # touched the config file (config_changed above); this avoids spurious - # permission errors when the config merely aged out of the cache - # we also give maintainers the option to force-set the webhook: if the - # commit message contains [hubcast config], we'll set the webhook + # only set/update the webhook on default branch pushes that touch the config + # file (avoids spurious permission errors on unrelated pushes); maintainers + # can also force it by including [hubcast config] in the commit message if is_default_branch and (config_changed or "[hubcast config]" in commit_msg): - # setup callback webhook on GitLab try: await gl.set_webhook( dest_org=repo_config.dest_org, @@ -385,12 +379,11 @@ async def remove_branch( *arg, **kwargs, ) -> None: - src_fullname = event.data["repository"]["full_name"] sync_ref = event.data["ref"] update_log_context(ref=sync_ref) - repo_config = await get_repo_config(gh, src_fullname) + repo_config = await get_repo_config(gh) dest_fullname = repo_config.dest_fullname dest_remote_url = f"{gl.instance_url}/{dest_fullname}.git" @@ -479,7 +472,7 @@ async def sync_pr( # get the repository configuration from .github/hubcast.yml try: - repo_config = await get_repo_config(gh, base_fullname) + repo_config = await get_repo_config(gh) except RepoConfigError as exc: # only report the default branch config's error when this push isn't trying to fix it if not config_changed: @@ -596,7 +589,7 @@ async def remove_pr( base_fullname = pull_request["base"]["repo"]["full_name"] # get the repository configuration from .github/hubcast.yml - repo_config = await get_repo_config(gh, base_fullname) + repo_config = await get_repo_config(gh) if not repo_config.delete_closed: log.info("Skipped PR branch removal - delete_closed disabled") @@ -711,7 +704,7 @@ async def respond_comment( update_log_context(branch=branch) # get the gitlab repo information and run the pipeline - repo_config = await get_repo_config(gh, base_fullname) + repo_config = await get_repo_config(gh) dest_fullname = repo_config.dest_fullname try: @@ -747,7 +740,7 @@ async def respond_comment( update_log_context(branch=branch) # get the gitlab repo information and run the pipeline - repo_config = await get_repo_config(gh, base_fullname) + repo_config = await get_repo_config(gh) dest_fullname = repo_config.dest_fullname try: @@ -831,7 +824,6 @@ async def rerun_check( Handles a user re-running a check run by retrying the specific GitLab job or pipeline it's attached to. See https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=rerequested#check_run. """ - src_fullname = event.data["repository"]["full_name"] check_run_commit = event.data["check_run"]["head_sha"] details_url = event.data["check_run"]["details_url"] update_log_context( @@ -848,7 +840,7 @@ async def rerun_check( return try: - repo_config = await get_repo_config(gh, src_fullname) + repo_config = await get_repo_config(gh) except RepoConfigError as exc: await report_config_error(gh, check_run_commit, exc) return diff --git a/src/hubcast/web/github/utils.py b/src/hubcast/web/github/utils.py index 16b80620..1eebdea6 100644 --- a/src/hubcast/web/github/utils.py +++ b/src/hubcast/web/github/utils.py @@ -3,7 +3,6 @@ import yaml import yaml.reader -from cachetools import TTLCache from pydantic import ValidationError from hubcast.clients.github import GitHubClient @@ -16,9 +15,6 @@ log = logging.getLogger(__name__) -# Shared cache for repository configs with 30-minute TTL -config_cache: TTLCache[str, RepoConfig | None] = TTLCache(maxsize=1000, ttl=1800) - def changed_files_from_push(payload: dict[str, Any]) -> set[str]: """Collect all file paths touched by the commits in a push payload.""" @@ -89,15 +85,15 @@ def parse_repo_config(raw_config: str) -> RepoConfig: ) -async def get_repo_config( - gh: GitHubClient, fullname: str, refresh: bool = False -) -> RepoConfig: - """Get repository configuration from cache or fetch from GitHub. +async def get_repo_config(gh: GitHubClient) -> RepoConfig: + """Fetch and validate the repository configuration from GitHub. + + Fetched fresh on every event so all replicas always see the current + destination repo; a single contents-API call is well within App + installation rate limits. Args: gh: GitHub client instance - fullname: Full repository name (e.g., "owner/repo") - refresh: Whether to force refresh from GitHub Returns: RepoConfig instance @@ -106,29 +102,16 @@ async def get_repo_config( HubcastError: If config file contains invalid YAML, is missing required keys, or has validation errors """ - # check cache first unless refresh is requested - if fullname in config_cache and not refresh: - config = config_cache[fullname] - log.info("Repo config retrieved from cache") - if config is None: - # raise so route handlers can't continue - # we don't want to raise this as a RepoConfigError because telling users - # about the absence of the config will create noise and confusion - raise HubcastError("Repo config file not found", log_level="INFO") - return config - - # cache miss or refresh requested, fetch from GH fetched_config = await gh.get_repo_config() if fetched_config is None: # 404 - config_cache[fullname] = None - log.info("Cached absence of repo config") # raise so route handlers can't continue + # we don't want to raise this as a RepoConfigError because telling users + # about the absence of the config will create noise and confusion raise HubcastError("Repo config file not found", log_level="INFO") # parse and validate YAML config = parse_repo_config(fetched_config) - config_cache[fullname] = config log.info("Repo config fetched from source forge") return config diff --git a/tests/test_github_routes.py b/tests/test_github_routes.py index f6570103..489dfc4d 100644 --- a/tests/test_github_routes.py +++ b/tests/test_github_routes.py @@ -421,38 +421,6 @@ async def test_sync_branch_object_present_but_ref_missing( mock_repligit_ops["send_pack"].assert_awaited_once() -@pytest.mark.asyncio -@pytest.mark.parametrize( - "ref,modified,expected_refresh", - [ - ("refs/heads/main", [".github/hubcast.yml"], True), - ("refs/heads/main", ["src/app.py"], False), - ("refs/heads/feature", [".github/hubcast.yml"], False), - ], -) -async def test_sync_branch_config_refresh( - ref, - modified, - expected_refresh, - mock_push_event, - mock_gh, - mock_gl, - mock_repligit_ops, -): - """Config should only be refreshed when a default branch push touches the config file.""" - - mock_push_event.data["ref"] = ref - mock_push_event.data["commits"] = [ - {"added": [], "modified": modified, "removed": []} - ] - - await sync_branch(event=mock_push_event, gh=mock_gh, gl=mock_gl, gl_user="gl-user") - - mock_repligit_ops["get_repo_config"].assert_awaited_once_with( - mock_gh, "owner/repo", refresh=expected_refresh - ) - - @pytest.mark.asyncio @pytest.mark.parametrize( "ref,modified,commit_msg,webhook_expected", diff --git a/tests/test_repo_config.py b/tests/test_repo_config.py index b0bd29e1..d7c415a5 100644 --- a/tests/test_repo_config.py +++ b/tests/test_repo_config.py @@ -6,11 +6,7 @@ from hubcast.exceptions import HubcastError, RepoConfigError from hubcast.repos.config import RepoConfig from hubcast.web.github.messages import CONFIG_INVALID_SUMMARY, CONFIG_INVALID_TITLE -from hubcast.web.github.utils import ( - changed_files_from_push, - config_cache, - get_repo_config, -) +from hubcast.web.github.utils import changed_files_from_push, get_repo_config ### FIXTURES @@ -24,14 +20,6 @@ def mock_github_client(): return client -@pytest.fixture(autouse=True) -def clear_config_cache(): - """Clear config cache before each test.""" - config_cache.clear() - yield - config_cache.clear() - - ### TESTS @@ -120,38 +108,20 @@ def test_validator_with_non_dict(): @pytest.mark.asyncio -async def test_get_repo_config_uses_cache(mock_github_client): - """Should use cached config on second call.""" - - # first call -- fetches from client - config1 = await get_repo_config(mock_github_client, "owner/repo") +async def test_get_repo_config(mock_github_client): + """Should fetch fresh config on every call.""" - # second call -- uses cache - config2 = await get_repo_config(mock_github_client, "owner/repo") - - assert config1.dest_org == config2.dest_org - mock_github_client.get_repo_config.assert_called_once() # should only be called once - - -@pytest.mark.asyncio -async def test_get_repo_config_refreshes(mock_github_client): - """Should refresh cache when requested.""" - - # first call - config1 = await get_repo_config(mock_github_client, "owner/repo") + config1 = await get_repo_config(mock_github_client) + assert config1.dest_org == "owner" # update mock to return different data mock_github_client.get_repo_config.return_value = ( "Repo:\n dest_org: new-org\n dest_name: new-repo\n" ) - # second call with refresh - config2 = await get_repo_config(mock_github_client, "owner/repo", refresh=True) - - assert config1.dest_org == "owner" + config2 = await get_repo_config(mock_github_client) assert config2.dest_org == "new-org" assert config2.dest_name == "new-repo" - # should be called twice due to refresh assert mock_github_client.get_repo_config.call_count == 2 @@ -171,6 +141,7 @@ async def test_get_repo_config_refreshes(mock_github_client): ), ], ) + async def test_get_repo_config_invalid_yaml(raw_config, expected_detail): """Test handling of invalid YAML in repo config.""" gh = AsyncMock() @@ -181,7 +152,7 @@ async def test_get_repo_config_invalid_yaml(raw_config, expected_detail): with pytest.raises( RepoConfigError, match="Invalid YAML in repo config" ) as exc_info: - await get_repo_config(gh, "owner/repo") + await get_repo_config(gh) # route handlers report these to the user via a failed check assert exc_info.value.title == CONFIG_INVALID_TITLE @@ -190,16 +161,13 @@ async def test_get_repo_config_invalid_yaml(raw_config, expected_detail): @pytest.mark.asyncio -async def test_get_repo_config_missing_repo_key(): +async def test_get_repo_config_missing_repo_key(mock_github_client): """Test handling of missing 'Repo' top-level key.""" - gh = AsyncMock() # valid YAML but missing the required 'Repo' key - gh.get_repo_config = AsyncMock(return_value="NotRepo:\n dest_org: owner\n") - gh.repo_owner = "owner" - gh.repo_name = "repo" + mock_github_client.get_repo_config.return_value = "NotRepo:\n dest_org: owner\n" with pytest.raises(RepoConfigError, match="Invalid repo config") as exc_info: - await get_repo_config(gh, "owner/repo") + await get_repo_config(mock_github_client) assert "top-level 'Repo' section" in exc_info.value.context["error"] assert exc_info.value.title == CONFIG_INVALID_TITLE @@ -209,47 +177,25 @@ async def test_get_repo_config_missing_repo_key(): @pytest.mark.asyncio -async def test_get_repo_config_missing_required_fields(): +async def test_get_repo_config_missing_required_fields(mock_github_client): """Test handling of missing required fields within Repo config.""" - gh = AsyncMock() # valid YAML with 'Repo' key but missing required fields - gh.get_repo_config = AsyncMock(return_value="Repo:\n dest_org: owner\n") - gh.repo_owner = "owner" - gh.repo_name = "repo" + mock_github_client.get_repo_config.return_value = "Repo:\n dest_org: owner\n" with pytest.raises(RepoConfigError, match="Invalid repo config") as exc_info: - await get_repo_config(gh, "owner/repo") + await get_repo_config(mock_github_client) assert "dest_name" in exc_info.value.summary assert "Field required" in exc_info.value.summary @pytest.mark.asyncio -async def test_get_repo_config_not_found_from_github(): +async def test_get_repo_config_not_found_from_github(mock_github_client): """Test handling when config file is not found on GitHub.""" - gh = AsyncMock() - gh.get_repo_config = AsyncMock(return_value=None) - - with pytest.raises(HubcastError, match="Repo config file not found"): - await get_repo_config(gh, "owner/repo") + mock_github_client.get_repo_config.return_value = None - -@pytest.mark.asyncio -async def test_get_repo_config_not_found_from_cache(): - """Test handling when cached config is None.""" - gh = AsyncMock() - gh.get_repo_config = AsyncMock(return_value=None) - - # first call caches None with pytest.raises(HubcastError, match="Repo config file not found"): - await get_repo_config(gh, "owner/repo") - - # second call should discover cache has None - with pytest.raises(HubcastError, match="Repo config file not found"): - await get_repo_config(gh, "owner/repo") - - # should only call GH once since second call used cache - gh.get_repo_config.assert_called_once() + await get_repo_config(mock_github_client) @pytest.mark.parametrize( From 8917a3d2562d23e397c656ba1ca7438f270db32c Mon Sep 17 00:00:00 2001 From: Alec Scott Date: Thu, 20 Aug 2026 13:34:01 -0700 Subject: [PATCH 2/5] Correctly depend on aiohttp functionality in gidgetlab Signed-off-by: Alec Scott --- pyproject.toml | 4 ++-- spack/repos/spack_repo/hubcast/packages/py_hubcast/package.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index d14b8344..5aa5e18d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,8 +8,8 @@ version = "0.0.1" dependencies = [ "aiohttp", "aiojobs", - "gidgethub", - "gidgetlab>=2.1.2", + "gidgethub[aiohttp]", + "gidgetlab[aiohttp]>=2.1.2", "pydantic", "pydantic-settings", "pyjwt", diff --git a/spack/repos/spack_repo/hubcast/packages/py_hubcast/package.py b/spack/repos/spack_repo/hubcast/packages/py_hubcast/package.py index 5a737c99..19c0524b 100644 --- a/spack/repos/spack_repo/hubcast/packages/py_hubcast/package.py +++ b/spack/repos/spack_repo/hubcast/packages/py_hubcast/package.py @@ -26,7 +26,7 @@ class PyHubcast(PythonPackage): depends_on("py-aiohttp", type=("build", "run")) depends_on("py-aiojobs", type=("build", "run")) depends_on("py-pyjwt", type=("build", "run")) - depends_on("py-gidgethub", type=("build", "run")) + depends_on("py-gidgethub+aiohttp", type=("build", "run")) depends_on("py-gidgetlab@2.1.2:+aiohttp", type=("build", "run")) depends_on("py-repligit", type=("build", "run")) depends_on("py-pyyaml", type=("build", "run")) From 74c73062c3363c55c9cb7cef1bf48bc85baf3096 Mon Sep 17 00:00:00 2001 From: Alec Scott Date: Thu, 20 Aug 2026 19:13:56 -0700 Subject: [PATCH 3/5] Remove unused variable Signed-off-by: Alec Scott --- src/hubcast/web/github/routes.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/hubcast/web/github/routes.py b/src/hubcast/web/github/routes.py index 92c0b2d7..d04f7d51 100644 --- a/src/hubcast/web/github/routes.py +++ b/src/hubcast/web/github/routes.py @@ -698,9 +698,7 @@ async def respond_comment( pull_request = await gh.get_pr(pr_number) # get the branch this PR belongs to - base_fullname = pull_request["base"]["repo"]["full_name"] branch = _pr_branch_name(pull_request) - update_log_context(branch=branch) # get the gitlab repo information and run the pipeline @@ -734,9 +732,7 @@ async def respond_comment( pull_request = await gh.get_pr(pr_number) # get the branch this PR belongs to - base_fullname = pull_request["base"]["repo"]["full_name"] branch = _pr_branch_name(pull_request) - update_log_context(branch=branch) # get the gitlab repo information and run the pipeline From 94be205ce23c62f31fa72288eb60f4ede8b3d043 Mon Sep 17 00:00:00 2001 From: Alec Scott Date: Wed, 26 Aug 2026 10:39:18 -0700 Subject: [PATCH 4/5] Simplify comments Signed-off-by: Alec Scott --- src/hubcast/web/github/utils.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/hubcast/web/github/utils.py b/src/hubcast/web/github/utils.py index 1eebdea6..c3463431 100644 --- a/src/hubcast/web/github/utils.py +++ b/src/hubcast/web/github/utils.py @@ -88,10 +88,6 @@ def parse_repo_config(raw_config: str) -> RepoConfig: async def get_repo_config(gh: GitHubClient) -> RepoConfig: """Fetch and validate the repository configuration from GitHub. - Fetched fresh on every event so all replicas always see the current - destination repo; a single contents-API call is well within App - installation rate limits. - Args: gh: GitHub client instance @@ -106,8 +102,10 @@ async def get_repo_config(gh: GitHubClient) -> RepoConfig: if fetched_config is None: # 404 # raise so route handlers can't continue + # # we don't want to raise this as a RepoConfigError because telling users - # about the absence of the config will create noise and confusion + # about the absence of the config will create noise and confusion as + # they may have installed the app but have not submitted a config file yet raise HubcastError("Repo config file not found", log_level="INFO") # parse and validate YAML From 2981f658e229e29c4c81d6a012b71774bc6d5711 Mon Sep 17 00:00:00 2001 From: Alec Scott Date: Wed, 26 Aug 2026 10:42:52 -0700 Subject: [PATCH 5/5] Remove whitespace in testing file Signed-off-by: Alec Scott --- tests/test_repo_config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_repo_config.py b/tests/test_repo_config.py index d7c415a5..3fe1fa87 100644 --- a/tests/test_repo_config.py +++ b/tests/test_repo_config.py @@ -141,7 +141,6 @@ async def test_get_repo_config(mock_github_client): ), ], ) - async def test_get_repo_config_invalid_yaml(raw_config, expected_detail): """Test handling of invalid YAML in repo config.""" gh = AsyncMock()