Factor out a function to load app config #519
Conversation
src/config/loadGitHubApps.ts
Outdated
| @@ -0,0 +1,14 @@ | |||
| import { AppAuthStrategyOptions } from '@api/github/appAuthStrategyOptions'; | |||
|
|
|||
| export function loadGitHubApps(env) { | |||
There was a problem hiding this comment.
does this function need to pass in environment variables as env? Can we just use process.env here?
There was a problem hiding this comment.
It makes testing much easier.
There was a problem hiding this comment.
In that case, can we just pass in GH_APP_IDENTIFIER and GH_APP_SECRET_KEY instead? feels a bit weird to me to pass all the env variables just to use two of them
There was a problem hiding this comment.
Filtering out unused envvars is part of the logic we want to test, no? Does da690d2 help? It changes the function name to loadGitHubAppsFromEnvironment and adds a few more tests.
There was a problem hiding this comment.
Also keep in mind that the next PR is going to add multi-org support, for which we're going to need to iterate over the whole process.env to pull out the vars we want.
There was a problem hiding this comment.
Gotcha, I think this is a pretty small thing and don't want to block you further. Thanks for making it more clear
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
==========================================
- Coverage 84.50% 84.48% -0.03%
==========================================
Files 96 97 +1
Lines 2479 2475 -4
Branches 481 473 -8
==========================================
- Hits 2095 2091 -4
Misses 377 377
Partials 7 7
☔ View full report in Codecov by Sentry. |
Part of #482.
Since we're going to have somewhat complex logic to parse envvars into app config, we should test as we go. Factoring out a function gives us something to test.