Skip to content

feat: implements config loader to enable remote or external configs #935

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

Merged
merged 3 commits into from
May 20, 2025

Conversation

dcoric
Copy link
Contributor

@dcoric dcoric commented Mar 10, 2025

This pull request introduces a significant update to the configuration management system of the GitProxy application. The main changes include adding support for multiple configuration sources, implementing a new configuration loader class, and providing the ability to reload configurations without restarting the application.

Configuration Management Enhancements:

  • Added support for multiple configuration sources in config.schema.json and proxy.config.json, allowing the configuration to be sourced from files, HTTP endpoints, or Git repositories. [1] [2]

  • Introduced a new ConfigLoader class in src/config/ConfigLoader.js to manage the loading and reloading of configurations from the specified sources. This class handles periodic reloading, merging configurations, and emitting events on configuration changes.

CLI and API Improvements:

  • Added a new CLI command reload-config in packages/git-proxy-cli/index.js to trigger a configuration reload without restarting the process. This command checks for authentication and makes a POST request to the admin API endpoint to reload the configuration. [1] [2]

  • Added a new admin-only API endpoint /api/v1/admin/reload-config in src/service/index.js to handle configuration reload requests. This endpoint validates the request, reloads the configuration, and restarts the services if necessary.

Proxy Service Enhancements:

  • Updated the proxy service in src/proxy/index.js to support stopping and restarting the HTTP and HTTPS servers, enabling the application to apply new configurations without a full restart.

Configuration Access and Updates:

  • Refactored src/config/index.js to use the new ConfigLoader and updated existing getter functions to retrieve values from the current configuration. Added functionality to handle configuration updates dynamically and restart services as needed. [1] [2] [3]fix: config loader clone command issue

This PR fixes #934

Copy link

linux-foundation-easycla bot commented Mar 10, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Mar 10, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit a4cfa78
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/682c7fc38c310c0008b6da6b
😎 Deploy Preview https://deploy-preview-935--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@JamieSlome JamieSlome requested a review from 06kellyjac March 15, 2025 11:09
@dcoric
Copy link
Contributor Author

dcoric commented Mar 17, 2025

I will rebase to the latest main branch, check the failing test, and post an update afterward.

Copy link
Contributor

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

Some suggestions and questions

@dcoric dcoric force-pushed the denis-coric/remote-config branch from aa0283d to 8d161a6 Compare March 25, 2025 13:57
Copy link

codecov bot commented Mar 31, 2025

Codecov Report

Attention: Patch coverage is 53.87755% with 113 lines in your changes missing coverage. Please review.

Project coverage is 48.51%. Comparing base (9a3fd30) to head (a4cfa78).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/config/ConfigLoader.ts 62.22% 59 Missing and 9 partials ⚠️
src/config/index.ts 34.61% 16 Missing and 1 partial ⚠️
src/proxy/index.ts 33.33% 14 Missing ⚠️
src/service/index.js 17.64% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #935      +/-   ##
==========================================
+ Coverage   48.09%   48.51%   +0.42%     
==========================================
  Files          49       51       +2     
  Lines        1813     2092     +279     
  Branches      196      241      +45     
==========================================
+ Hits          872     1015     +143     
- Misses        917     1040     +123     
- Partials       24       37      +13     

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

@JamieSlome
Copy link
Member

@dcoric - can we bolster up the code coverage? 🚀

@JamieSlome JamieSlome self-requested a review March 31, 2025 12:51
@dcoric dcoric force-pushed the denis-coric/remote-config branch from 7dea019 to 55ce8ee Compare April 2, 2025 14:37
@dcoric
Copy link
Contributor Author

dcoric commented Apr 2, 2025

@dcoric - can we bolster up the code coverage? 🚀

Done.
This helped to identify (and fix) some potential issues with validation functions 👏

@JamieSlome
Copy link
Member

@dcoric - thanks for attending the call today! As discussed, let's take a look at the failing CI and we can get this merged 👍

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

feat: converted to typescript

fix: config loader clone command issue

fix: adds input validation, uses array arguments, prevented shell spawn

fix: adds failsafe checking for directory location and structure

fix: env-paths change to v2.2.1 which support require and minor code fix

fix: improves test coverage

Adds additional tests for better cove

fix: fixed creating cache directory
@JamieSlome
Copy link
Member

@dcoric - are you seeing merge conflicts?

Default value should be false, and set to true when cert path is configured
@dcoric
Copy link
Contributor Author

dcoric commented May 20, 2025

@dcoric - are you seeing merge conflicts?

Yes, on it right now.

@dcoric dcoric force-pushed the denis-coric/remote-config branch from 41697aa to a4cfa78 Compare May 20, 2025 13:12
@JamieSlome JamieSlome removed the request for review from 06kellyjac May 20, 2025 14:46
@JamieSlome JamieSlome dismissed 06kellyjac’s stale review May 20, 2025 14:47

Review on missing commits.

@JamieSlome JamieSlome enabled auto-merge May 20, 2025 14:48
@JamieSlome JamieSlome merged commit fcc5a4c into finos:main May 20, 2025
14 checks passed
@@ -648,5 +648,105 @@ description: JSON schema reference documentation for GitProxy
</blockquote>
</details>

----------------------------------------------------------------------------------------------------------------------------
<details>
<summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

@dcoric was this content written manually? Its supposed to be generated from the config schema, but I'm not seeing matching info added to the schema above. Hence, it will all be lost the next time that the ref doc is re-generated (npm run gen-schema-doc - requires you to have a python env setup for it to run in). I'm doing that on another PR that adds to the config schema so that will happen soon... A new PR is probably needed to correct this @JamieSlome @sam-holmes2

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