feat: allow ports to be customised via env vars

[maoo]

Closes #378

To test this PR:

npm install -g
export GIT_PROXY_UI_PORT="5000"
export GIT_PROXY_SERVER_PORT="9090"
git-proxy

You should see:

Service Listening on 5000
Listening on 9090

[netlify]

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name	Link
🔨 Latest commit	8d0ec57
🔍 Latest deploy log	https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/65a1168b5c01ce0008c8347d

[lwhiteley]

from the changes it looks good,
i dont see any reason why it wouldnt work.

I wont be able to test it until next week either way. [reviewing from phone]

maybe someone else can test to not hold it up

[lwhiteley]

Given there are more than 2 files affected here, it may be better to create a file to setup the environment variables and their defaults to avoid future mistakes

for eg.

// file path: src/common/env.js

const {
  GIT_PROXY_SERVER_PORT = 8000,
  GIT_PROXY_UI_PORT = 8080
} = process.env

export { GIT_PROXY_SERVER_PORT, GIT_PROXY_UI_PORT}

then here:

Suggested change

	const proxyHttpPort = process.env.GIT_PROXY_SERVER_PORT || 8000;
	const { GIT_PROXY_SERVER_PORT: proxyHttpPort } = require('../common/env')

similar for other files.

this way the defaults are only defined in one place and easier to maintain.

you can then further use this when reading the file config somehow later given its all in one place

[lwhiteley]

[optional]

maybe also add an example of how to set the env vars via command line

[maoo]

@lwhiteley - all good suggestions, thanks! I've pushed changes following your suggestions.

@JamieSlome @coopernetes - please review/text at your earliest convenience. TY!

[coopernetes]

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.

[JamieSlome]

Can we look to include some test functionality to demonstrate the usage and successful loading of ports via ENV?

[JamieSlome]

Nice PR! 👍

I've made a couple of comments and mentioned in a previous commit that a test case would be great prior to merge.

Thank you @maoo @lwhiteley 🍰

[maoo]

Added tests, but when testing custom ports, the test fails; I think that the problem is how I'm stubbing environment variables in testConfig.js

[maoo]

Thanks @lwhiteley - that's the same code I did, and yes, the tutorial I followed used Jest; I didn't really think that mocha would not work.

Re. e2e testing, I agree with you, but the issue with env var setup would still be there.

I've tried several approaches, including https://medium.com/nodejsmadeeasy/elegant-ways-to-pass-env-variables-to-mocha-test-cases-4486cb238bb1 and https://glebbahmutov.com/blog/mocking-process-env/

I'll sleep on it and see if next week I can find a working solution. @JamieSlome - did you face this issue in the past with mocha? And is there any e2e testing where we can add the custom port testing?

[lwhiteley]

Yup but I don't mean in a spec runner like jest or mocha where you have to manipulate the require cache.

It may be possible to setup jobs that simulate how the end user would test it in CI and just provide the env vars there to confirm by pinging the running services.

[maoo]

I see that @coopernetes raised #394 - so maybe we could simply skip the e2e part on this PR and tackle this challenge in a separate issue/PR ? WDYT?

[JamieSlome]

@maoo - sounds good 👍 Feel free to remove the test(s) and we can get this merged 🍰

[JamieSlome]

Ready to merge 👍

[maoo]

Thanks for stepping in @JamieSlome !