feat(config): multi-GitHub-App support + ResolveGitHubAppForRepo#215
Conversation
Add an additive apps: map under ServerConfig and a per-repo github_app: reference so one SchemaBot process can register N GitHub Apps and route each repository to the App that owns it. Mutually exclusive with the legacy single-App github: field; legacy shape is unchanged. The resolver and types land in this PR with no runtime consumers — a follow-up PR will wire the webhook dispatcher, ClientSet, and recovery path. Docs and the Helm values example flag the wiring as deferred. Refs #204.
There was a problem hiding this comment.
Pull request overview
Adds an additive multi-GitHub-App configuration surface so a single SchemaBot process can register multiple Apps and route each repository to the App that owns it. The legacy single-App github: shape is preserved unchanged and is mutually exclusive with the new apps: map. A new ResolveGitHubAppForRepo resolver provides a uniform contract for downstream callers; webhook dispatch wiring is deferred to a follow-up PR.
Changes:
- Add
ServerConfig.Apps(map ofGitHubAppConfig, aliased toGitHubConfig) andRepoConfig.GitHubApp, with mutual-exclusivity and per-app/per-repo validation rules. - Introduce
ResolvedGitHubAppandServerConfig.ResolveGitHubAppForRepo, returning the legacy App under the synthetic name"default". - Document the new shape in
docs/configuration.mdand the Helmvalues.yamlexample, with comprehensive table-driven tests for validation, resolution, and YAML round-trip.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/api/config.go | New Apps field, GitHubAppConfig alias, RepoConfig.GitHubApp, validation, and ResolveGitHubAppForRepo. |
| pkg/api/config_test.go | Validation matrix, resolver behavior tests, and end-to-end YAML parse test. |
| docs/configuration.md | New "Multi-App Routing" section with rules and example. |
| charts/schemabot/values.yaml | Commented example of the multi-App config under Helm values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous wording said GitHubApp was 'ignored when only the legacy single-App GitHub field is set', but validation actually rejects that combination to fail closed on misconfiguration. Update the comment to match the behavior.
Resolves conflicts in charts/schemabot/Chart.yaml and charts/schemabot/values.yaml introduced by #213 (multi-deployment config) landing on main. - Chart.yaml: kept 0.1.6 (one above main's current 0.1.5). - values.yaml: kept both example blocks — multi-deployment first (established on main), then this PR's multi-App example.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd8a442273
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When a config sets apps: only (no legacy github: block), validation used to succeed but buildWebhookRuntime would still install the disabled 503 webhook handler — so every accepted multi-App config started a server with a silently-broken /webhook endpoint. Fix: extend validateGitHubAppsConfig to return an explicit "apps: is configured but the webhook runtime does not yet route to it" error after the per-entry / per-repo checks pass. Mirrors the fail-closed pattern already used for multi-deployment configs. Test updates: - Convert the two apps: happy-path cases in TestServerConfig_AppsValidation (multi-app, single-entry) to assert the new hard-block error. - Refactor TestLoadServerConfigFromFile_MultiApp into a parse-only round-trip (still verifies YAML field tags via yaml.NewDecoder with KnownFields(true)) plus a LoadServerConfigFromFile step that now asserts the fail-closed error. Docs: - docs/configuration.md: section header reframed to "Multi-App Routing (PREVIEW — config rejected at load)"; replaced the misleading "accepted by Validate()" note with an upfront PREVIEW callout; rules list reframed as "enforced once the runtime is wired". - charts/schemabot/values.yaml: example block flagged PREVIEW.
What
Adds an additive
apps:map underServerConfigand a per-repogithub_app:reference so one SchemaBot process can register N GitHub Apps and route each repository to the App that owns it. Mutually exclusive with the legacy single-Appgithub:field; legacy shape is unchanged.Why
Multi-tenant deployments (one App per org, or multiple Apps per org with strict isolation) currently require running N SchemaBot processes — N webhook endpoints, N deployments, N metrics streams. This lands the config + resolver contract so a follow-up PR can be a pure consumer of the new API when wiring the webhook dispatcher.
Config shape
Validation rules
github:andapps:are mutually exclusive.apps:is set, every entry inrepos:MUST setgithub_app:to one of the configured app names.apps:is NOT set, repos MUST NOT setgithub_app:(rejected explicitly — no silent ignore).app-id,private-key, andwebhook-secret.apps: {}is rejected.Resolver
Multi-App configs resolve to the named App; the legacy
github:shape resolves under the synthetic name"default"so downstream callers can treat both shapes uniformly.Out of scope
X-GitHub-Hook-Installation-Target-IDrouting, per-App HMAC verification).ClientSetand per-App installation token minting inpkg/github.serve.goand recovery path wiring.All deferred to the follow-up PR that wires the runtime to consume
ResolveGitHubAppForRepo.Chart Version
The charts/schemabot/Chart.yaml bump is deferred to just before merge so this PR doesn't conflict with the chart bump in #213 (whichever lands second will bump).
Rollback
Pure revert. No on-disk artifact, no schema changes, no live callers of the new resolver.
Refs #204.