-
Notifications
You must be signed in to change notification settings - Fork 1
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
DM-44635: Nicer GitHub integration config #354
DM-44635: Nicer GitHub integration config #354
Conversation
2a9910f
to
46c11f7
Compare
46c11f7
to
61300ac
Compare
61dbc2c
to
32a11e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great.
This path looks familiar from Gafaelfawr: I went down the path of having configuration options giving the path to a YAML file for a while, and it works fine. In the long run, though, I decided to do the work that you suggest in the PR description and unify all the configuration files into a single YAML file that was just a verbatim dump of the config
key of the Helm chart, and then only use environment variables for the secrets that don't come from the values.yaml
file. I ended up liking that a lot better because it meant there's only one configuration language (well, plus the stuff that has to be environment variables), and I never had to mentally translate between what stuff looked like in Helm and what stuff looked like in the app.
I completely agree that we can kick this can down the road, though.
…ng yaml file configs
8737789
to
9a16f5b
Compare
9a16f5b
to
ee9f4b4
Compare
Split out GitHub CI and refresh app config
GitHub CI and refresh app config are now each a separate, all-or-nothing set of config that comes from a mix of a yaml file and env vars.
If a path to one of these GitHub app config files is given in the base config, then:
lifespan
functionHaving the whole set of config be required, rather than each config item being optional makes app initialization much less complex.
Config now comes from a mix of environment variables and multiple
ConfigMap
s (GitHub app integrations and autostart config). It may be worth looking at smooshing it all into one singleConfigMap
in the future (or simplifying it some other way), but that would involve a lot of Mobu and Phalanx changes, and I think we can kick it down the road for now.This is supported by these phalanx changes.
Dynamic GitHub CI app Gafaelfawr scopes
The scopes granted to GitHub CI app jobs now come from config and can be set differently per environment in Phalanx.