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..5aa5e18d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,9 +8,8 @@ version = "0.0.1" dependencies = [ "aiohttp", "aiojobs", - "cachetools", - "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")) diff --git a/src/hubcast/web/github/routes.py b/src/hubcast/web/github/routes.py index d2122aae..d04f7d51 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") @@ -705,13 +698,11 @@ 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 - repo_config = await get_repo_config(gh, base_fullname) + repo_config = await get_repo_config(gh) dest_fullname = repo_config.dest_fullname try: @@ -741,13 +732,11 @@ 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 - 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 +820,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 +836,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..c3463431 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,11 @@ 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. 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 +98,18 @@ 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 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 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..3fe1fa87 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") - - # 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.""" +async def test_get_repo_config(mock_github_client): + """Should fetch fresh config on every call.""" - # 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 @@ -181,7 +151,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 +160,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 +176,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(