Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: simplify syncRepos call in push to resolve trpc issue #321

Conversation

riley-kohler
Copy link
Contributor

@riley-kohler riley-kohler commented Feb 28, 2025

Pull Request

Proposed Changes

Our team has been facing issues with syncing not working on our deployed version of private-mirrors. Everything works locally but when deployed the syncing fails with a TRPCClientError that has no further details. Every other feature of private-mirrors is working from what we can tell and we are unsure if this issue may be affecting other private-mirrors installations. Through investigation, we attempted to get any meaningful error or debug log output but were unable to get anything helpful.

After taking a more in depth look at the implementation, we noticed that the sync functionality was the only trpc call made from the server rather than from a client browser and it seemed that there isn't much benefit to calling the sync function this way. Instead, there is added complexity by going through trpc that could be avoided by calling the function directly. This PR modifies the push handling of private-mirrors to call the syncReposHandler directly instead of going through trpc which resolves the issue that we were encountering and reduces complexity.

While it would be nice to isolate the original issue to prevent any similar future problems, this change effectively resolves the issue and even if the issue is not present in other deployments in our opinion should be merged to reduce the likelihood over other issues arising.

Any feedback would be greatly appreciated, this code is also the only area where the serverTrpc is used so that related code can be removed if desired unless it is intended to be used for another purpose. It's likely that any similar usage of serverTrpc will cause the same issue.

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run npm run lint and fix any linting issues that have been introduced
  • run npm run test and run tests
  • If publishing new data to the public (scorecards, security scan results, code quality results, live dashboards, etc.), please request review from @jeffrey-luszcz

Reviewer

  • Label as either bug, documentation, enhancement, infrastructure, maintenance, or breaking

@riley-kohler riley-kohler requested review from a team as code owners February 28, 2025 14:57
@github-actions github-actions bot added the fix label Feb 28, 2025
@Miablo Miablo enabled auto-merge March 3, 2025 16:27
Copy link
Contributor

@Miablo Miablo left a comment

Choose a reason for hiding this comment

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

🚢

@Miablo Miablo merged commit 0256691 into github-community-projects:main Mar 3, 2025
11 checks passed
@Miablo Miablo deleted the simplify_syncRepos_call_for_push branch March 3, 2025 16:32
@ahpook
Copy link
Contributor

ahpook commented Mar 3, 2025

Thanks, this LGTM and makes sense - I think there may have been some ideas for additional serverTrpc usage but addressing issues that come from actual usage is more important than a working around hypothetical future problem :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants