Skip to content

feat(key on repo url): support git hosts other than GitHub + multiple forks #1043

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

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

kriswest
Copy link
Contributor

@kriswest kriswest commented Jun 3, 2025

resolves #950
resolves #511
resolves #66

Refactor (api, proxy & UI) to remove the assumption of GitHub as the git repository host and the use of the repository name field as the id of the repository (as this prevents git-proxy instances from supporting multiple forks of a project or projects from multiple hosts with the same name).

Based on #979 - I'll rebase after that merges which should drop the number of affected files down some.

This PR:

  • Replaces the use of the repo name field in the API with the _id field generated by the database adaptors,
    • Using the repository URL as a key does not work well with express routing, but _id does in both mongo and neDb
    • allows names to be repeated (multiple forks or clashing names from different organisations/repository hosts)
    • UI and CLI were updated accordingly
  • Replaces the use of organisation/repoName.git in the proxy URLs with the repository url
  • Disables GitHub specific functionality in the UI if the host is not Github
  • Completes application of Typescript to the database classes
    • Duplicated code reduced
    • A number of minor differences in behaviour (particularly return types) between the DB adaptors were resolved
    • Does NOT refactor all usages of the DB client to use typescript (still many requires to eliminate)
  • Deprecates and ignores the config property proxyUrl as the proxied host(s) are now determined from the configured repositories

To Do:

  • Annotate PR for review
  • Check test coverage
  • Implement additional tests for the proxy and fallback
    • implement tests for new proxy URLs for github.com
    • implement tests for fallback with legacy proxy urls for github.com
    • implement tests for gitlab.com
    • implement tests for non-github/non-gitlab repo
    • implement tests for multiple forks
  • Add support for GitLab API where repo is hosted at GitLab

(contributed as part of a GitLab CoCreate collaboration with help from @StingRayZA)

kriswest and others added 30 commits May 19, 2025 16:58
Typescript wasn't working on the DB classes due to their dependency imports with require.
StingRayZA and others added 16 commits June 3, 2025 14:17
Typescript wasn't working on the DB classes due to their dependency imports with require.
Copy link

netlify bot commented Jun 3, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit f3de59c
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6841d39c9f976d000832caf3

Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 60.15625% with 153 lines in your changes missing coverage. Please review.

Project coverage is 66.88%. Comparing base (67663a9) to head (f3de59c).

Files with missing lines Patch % Lines
src/proxy/routes/index.ts 20.68% 46 Missing ⚠️
src/db/mongo/repo.ts 37.14% 22 Missing ⚠️
src/proxy/routes/helper.ts 51.28% 16 Missing and 3 partials ⚠️
src/db/types.ts 13.33% 13 Missing ⚠️
src/proxy/processors/pre-processor/parseAction.ts 13.33% 13 Missing ⚠️
src/db/mongo/users.ts 40.00% 12 Missing ⚠️
src/db/index.ts 90.47% 7 Missing and 1 partial ⚠️
src/proxy/actions/Action.ts 71.42% 2 Missing and 2 partials ⚠️
src/service/routes/repo.js 78.94% 4 Missing ⚠️
src/db/file/repo.ts 92.50% 2 Missing and 1 partial ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
- Coverage   70.29%   66.88%   -3.42%     
==========================================
  Files          54       61       +7     
  Lines        2205     2449     +244     
  Branches      248      287      +39     
==========================================
+ Hits         1550     1638      +88     
- Misses        624      776     +152     
- Partials       31       35       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@sam-holmes2 sam-holmes2 left a comment

Choose a reason for hiding this comment

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

LGTM after an initial scan through :) thanks for your contribution!

@@ -578,20 +578,24 @@ export default function Repositories(props) {
getGitHubRepository();
}, [props.data.project, props.data.name]);

// TODO add support for GitLab API: https://docs.gitlab.com/api/projects/#get-a-single-project

Choose a reason for hiding this comment

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

Flagging for awareness

const router = Router();

getAllProxiedHosts().then((originsToProxy) => {
// TODO: this will only happen on startup. We'll need to add routes at runtime when new origins are added? Or force a restart for the proxy to work

Choose a reason for hiding this comment

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

Flagging for awareness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was reading up on a solution for this... as we're using a router it can be reinitialized. The trick will be figuring out how to tell it to do so!

@kriswest
Copy link
Contributor Author

kriswest commented Jun 5, 2025

Picked up a couple of test failures after merging main - will resolve (and start working on the additional tests needed).

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