Skip to content

refactor: git-proxy-cli package to TS + ESM (v2)#1180

Merged
kriswest merged 27 commits intofinos:mainfrom
jescalada:refactor-cli-to-ts-redone
Oct 14, 2025
Merged

refactor: git-proxy-cli package to TS + ESM (v2)#1180
kriswest merged 27 commits intofinos:mainfrom
jescalada:refactor-cli-to-ts-redone

Conversation

@jescalada
Copy link
Contributor

@jescalada jescalada commented Aug 30, 2025

Fixes #1064. Replaces #1065 due to merge conflicts.

I modified some things such as the CLI execution script because the original one (npx -- @finos/git-proxy-cli), oddly, no longer works.

I'd love to know better ways to handle the build/testing step and if the CLI flow works as it used to!

@netlify
Copy link

netlify bot commented Aug 30, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 27c604c
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68ee50ca1bb68900085646e8

@codecov
Copy link

codecov bot commented Aug 30, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.07%. Comparing base (84d2563) to head (27c604c).
⚠️ Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
src/db/mongo/pushes.ts 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1180      +/-   ##
==========================================
- Coverage   84.09%   84.07%   -0.03%     
==========================================
  Files          68       68              
  Lines        2943     2945       +2     
  Branches      374      375       +1     
==========================================
+ Hits         2475     2476       +1     
- Misses        408      409       +1     
  Partials       60       60              

☔ 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.

@jescalada jescalada marked this pull request as ready for review August 30, 2025 13:14
@jescalada jescalada linked an issue Aug 31, 2025 that may be closed by this pull request
@jescalada jescalada requested a review from a team August 31, 2025 10:15
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Again, great work, happy to see this happen.

I think we should use the same types in the API and CLI, rather than creating additional copies. If we use different copies of type definitions between them, then changes in the API will not be obvious when working on the CLI, whereas, with common types the fields available are clear and changes (say to remove a field that is depended on in the CLI) will break the build (as it should).

Copy link
Contributor

@dcoric dcoric left a comment

Choose a reason for hiding this comment

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

I did some testing of the CLI functionality, and it is good. TS is compiled to JS on build and publish. The only thing that I had to update on my end to make it run is in package.json on the root project:

  "scripts": {
    "cli": "tsx ./packages/git-proxy-cli/index.ts",
    "cli:js": "node ./packages/git-proxy-cli/dist/index.js",

To run the JS version, it has to happen from the dist folder, and it is possible to run the TS version with the tsx command.

@jescalada
Copy link
Contributor Author

@kriswest This should be ready for a final check - looking to merge this ASAP as it's blocking some of our team's PRs 🙏🏼

@kriswest kriswest self-requested a review September 22, 2025 15:15
@jescalada jescalada requested a review from 06kellyjac October 1, 2025 04:34
@jescalada
Copy link
Contributor Author

@kriswest Seems this PR requires a re-review on your end before merging! 🙂

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Mostly looks good, flagged one issue in the test utils/test where I think the type is wrong

@jescalada jescalada requested a review from kriswest October 10, 2025 11:12
@jescalada
Copy link
Contributor Author

@kriswest Added a temporary fix for the unknowns on the CLI tests. @dcoric is working on refactoring them to Vitest so most of this will be subject to change soon 😃

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM

#1166 to update typing in packages/git-proxy-cli/test/testCliUtils.ts startServer

@kriswest kriswest enabled auto-merge October 14, 2025 13:34
@kriswest kriswest merged commit 34d87a0 into finos:main Oct 14, 2025
14 checks passed
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.

Refactor CLI script to TS + ESM Convert CLI to typescript

4 participants