Skip to content
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

Merged
merged 11 commits into from
Jul 11, 2024

Conversation

fajpunk
Copy link
Member

@fajpunk fajpunk commented Jul 10, 2024

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:

  • All of that particular config is validated and loaded
  • Any necessary initialization is done in the lifespan function
  • Handlers for that functionality are mounted in the app

Having 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 ConfigMaps (GitHub app integrations and autostart config). It may be worth looking at smooshing it all into one single ConfigMap 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.

@fajpunk fajpunk force-pushed the tickets/DM-44635/nicer-github-integration-config branch 5 times, most recently from 2a9910f to 46c11f7 Compare July 11, 2024 01:27
@fajpunk fajpunk force-pushed the tickets/DM-44635/nicer-github-integration-config branch from 46c11f7 to 61300ac Compare July 11, 2024 03:37
@fajpunk fajpunk requested a review from rra July 11, 2024 14:23
@fajpunk fajpunk mentioned this pull request Jul 11, 2024
@fajpunk fajpunk force-pushed the tickets/DM-44635/nicer-github-integration-config branch 2 times, most recently from 61dbc2c to 32a11e7 Compare July 11, 2024 15:10
Copy link
Member

@rra rra left a 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.

docs/operations/github_ci_app.rst Outdated Show resolved Hide resolved
docs/operations/github_ci_app.rst Outdated Show resolved Hide resolved
docs/operations/github_ci_app.rst Outdated Show resolved Hide resolved
src/mobu/dependencies/github.py Outdated Show resolved Hide resolved
src/mobu/github_config.py Outdated Show resolved Hide resolved
src/mobu/github_config.py Outdated Show resolved Hide resolved
src/mobu/github_config.py Show resolved Hide resolved
src/mobu/services/github_ci/ci_manager.py Show resolved Hide resolved
@fajpunk fajpunk force-pushed the tickets/DM-44635/nicer-github-integration-config branch 2 times, most recently from 8737789 to 9a16f5b Compare July 11, 2024 20:26
@fajpunk fajpunk force-pushed the tickets/DM-44635/nicer-github-integration-config branch from 9a16f5b to ee9f4b4 Compare July 11, 2024 20:47
@fajpunk fajpunk merged commit 50e4045 into main Jul 11, 2024
4 checks passed
@fajpunk fajpunk deleted the tickets/DM-44635/nicer-github-integration-config branch July 11, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants