Skip to content

Consolidate sync actions in route handlers#240

Open
cmelone wants to merge 3 commits intollnl:mainfrom
cmelone:add/consolidate-syncs
Open

Consolidate sync actions in route handlers#240
cmelone wants to merge 3 commits intollnl:mainfrom
cmelone:add/consolidate-syncs

Conversation

@cmelone
Copy link
Member

@cmelone cmelone commented Mar 4, 2026

There was a lot of duplicated code/logic in the route handlers, especially for the sync/delete ref actions. I was a bit aggressive with the consolidation, so let me know if you'd like to keep something expanded.

NOTE:

  • when should refresh=True when calling get_repo_config?
  • apologies in advance for the atrocious diff
  • the gitlab webhook is now created/managed in the shared sync helper rather than exclusively in the push handler

@cmelone cmelone requested a review from Copilot March 4, 2026 23:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR consolidates duplicated logic in GitHub route handlers by extracting common sync, delete, and pipeline context operations into shared helper functions. The RepoConfig class is also updated to compute fullname automatically.

Changes:

  • Extracted _sync_refs, _delete_ref, _get_dest_context, _get_pr_target_ref, and _get_pr_pipeline_context as shared helpers to eliminate duplicated code across push and pull request event handlers.
  • Simplified sync_pr signature by removing the src_repo_private parameter (now derived from the pull_request dict directly).
  • Changed RepoConfig.fullname to be auto-computed from dest_org and dest_name instead of being passed as a separate parameter.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/hubcast/web/github/routes.py Route handlers refactored to use new helper functions, removing significant code duplication
src/hubcast/repos/config.py RepoConfig.fullname now auto-computed from dest_org/dest_name

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cmelone added a commit to cmelone/hubcast that referenced this pull request Mar 5, 2026
Hubcast config settings for repos should only be fetched from the
default branch of the base repository. Previously, we were getting the
config from the head repo, which would lead to potential confusion for
users submitting PRs via forks.

This ensures that there is one source of settings for each repository.

I pulled this out of llnl#240 to make for a faster review.
alecbcs pushed a commit that referenced this pull request Mar 6, 2026
Hubcast config settings for repos should only be fetched from the
default branch of the base repository. Previously, we were getting the
config from the head repo, which would lead to potential confusion for
users submitting PRs via forks.

This ensures that there is one source of settings for each repository.

I pulled this out of #240 to make for a faster review.
@cmelone cmelone force-pushed the add/consolidate-syncs branch 5 times, most recently from 9e8e2e5 to c9ac693 Compare March 11, 2026 21:15
@cmelone cmelone marked this pull request as ready for review March 11, 2026 21:16
There was a lot of duplicated code/logic in the route handlers,
especially for the sync/delete ref actions. I was a bit aggressive with
the consolidation, so let me know if you'd like to keep something
expanded.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +60 to +62
async def _get_dest_context(repo_fullname, gh, gl, gl_user):
"""Returns common destination repo details needed for sync/delete operations."""
repo_config = await get_repo_config(gh, repo_fullname, refresh=True)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

In the sync_pr flow, get_repo_config is called twice: once here (without refresh) for the draft check, and again inside _sync_refs_get_dest_context (with refresh=True). Consider passing the already-fetched repo_config into _sync_refs to avoid the redundant network call, or at minimum make the refresh parameter in _get_dest_context configurable rather than always True.

Suggested change
async def _get_dest_context(repo_fullname, gh, gl, gl_user):
"""Returns common destination repo details needed for sync/delete operations."""
repo_config = await get_repo_config(gh, repo_fullname, refresh=True)
async def _get_dest_context(repo_fullname, gh, gl, gl_user, *, refresh: bool = True, repo_config=None):
"""Returns common destination repo details needed for sync/delete operations."""
if repo_config is None or refresh:
repo_config = await get_repo_config(gh, repo_fullname, refresh=refresh)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants