git-proxy icon indicating copy to clipboard operation
git-proxy copied to clipboard

feat: allow git-proxy to be configured both by environment as well as config file(s)

Open coopernetes opened this issue 1 year ago • 3 comments

Tracking overall enhancements to how the git-proxy application receives its user configurable values.

### Tasks
- [ ] Allow all user settings to be defined in a JSON file (largely complete but remove any hard-coded values from src)
- [ ] Allow `GIT_PROXY_*` environment variables to override file-based settings
- [ ] Add test cases to configuration parsing for all settings (improve coverage)
- [ ] Adopt a [proper library to handle externalized config](https://www.npmjs.com/package/config)

LGTM - as a future enhancement, we should expose the server ports as both file-based config (via proxy.config.json) as well as environment variables. We should have this available for all our configurable values, merge or reconcile between the two sources and allow users to choose whichever method best suits their deployment to configure ports & other values.

Originally posted by @coopernetes in https://github.com/finos/git-proxy/pull/379#pullrequestreview-1802800723

coopernetes avatar Jan 03 '24 17:01 coopernetes

This makes sense. I'd prefer personally customizing via the proxy.config.json but to have the functionality for both is very sensical 👍

Can we just get some minimal requirements or a small tasklist for this issue? Sorting out #384 at the moment, then will get #213 merged, so we can start enforcing code coverage on new functionality, so please also include test cases as a requirement.

As always, thank you @coopernetes ❤️

JamieSlome avatar Jan 05 '24 13:01 JamieSlome

Done. I think using a full-fledged configuration library like node-config would cut down on a lot of boilerplate and we can likely remove the requirements for increasing test coverage if a library can handle the merging between sources (similar to how Spring Boot external config works).

coopernetes avatar Jan 07 '24 17:01 coopernetes

@coopernetes - agreed. If we can get the functionality with test coverage elsewhere that's great.

Also want us to be mindful of dependency bloat. Once we've had some proper architecture conversations post reconcile, we'll address our current dependency bloat in any case.

JamieSlome avatar Jan 11 '24 08:01 JamieSlome