Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/requirements/unit-tests.txt
Original file line number Diff line number Diff line change
@@ -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
Expand Down
5 changes: 2 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down
32 changes: 10 additions & 22 deletions src/hubcast/web/github/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,14 +279,11 @@ 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}"
config_changed = gh.repo_config_path in changed_files_from_push(event.data)
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:
await report_config_error(gh, want_sha, exc)
return
Expand All @@ -296,13 +293,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,
Expand Down Expand Up @@ -369,12 +363,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"
Expand Down Expand Up @@ -420,7 +413,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:
await report_config_error(gh, want_sha, exc)
return
Expand Down Expand Up @@ -531,7 +524,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")
Expand Down Expand Up @@ -640,13 +633,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:
Expand Down Expand Up @@ -676,13 +667,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:
Expand Down Expand Up @@ -766,7 +755,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(
Expand All @@ -783,7 +771,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
Expand Down
33 changes: 8 additions & 25 deletions src/hubcast/web/github/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from typing import Any

import yaml
from cachetools import TTLCache

from hubcast.clients.github import GitHubClient
from hubcast.exceptions import HubcastError, RepoConfigError
Expand All @@ -14,9 +13,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."""
Expand All @@ -28,15 +24,15 @@ def changed_files_from_push(payload: dict[str, Any]) -> set[str]:
}


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.
Comment on lines +30 to +32

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be simplified, not sure if the last clause is needed in the codebase


Args:
gh: GitHub client instance
fullname: Full repository name (e.g., "owner/repo")
refresh: Whether to force refresh from GitHub

Returns:
RepoConfig instance
Expand All @@ -45,24 +41,12 @@ 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
Comment on lines +48 to +49

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-- because 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
Expand All @@ -86,6 +70,5 @@ async def get_repo_config(
error=str(e),
)

config_cache[fullname] = config
log.info("Repo config fetched from source forge")
return config
32 changes: 0 additions & 32 deletions tests/test_github_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -414,38 +414,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",
Expand Down
Loading
Loading