Skip to content

feat(config): multi-GitHub-App support + ResolveGitHubAppForRepo#215

Merged
Kiran01bm merged 5 commits into
mainfrom
kiran01bm/multi-github-app-config-schema
Jun 2, 2026
Merged

feat(config): multi-GitHub-App support + ResolveGitHubAppForRepo#215
Kiran01bm merged 5 commits into
mainfrom
kiran01bm/multi-github-app-config-schema

Conversation

@Kiran01bm
Copy link
Copy Markdown
Collaborator

@Kiran01bm Kiran01bm commented Jun 2, 2026

What

Adds 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.

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

apps:
  app-a:
    app-id: "env:APP_A_ID"
    private-key: "file:/secrets/app-a/private-key"
    webhook-secret: "file:/secrets/app-a/webhook-secret"
  app-b:
    app-id: "env:APP_B_ID"
    private-key: "file:/secrets/app-b/private-key"
    webhook-secret: "file:/secrets/app-b/webhook-secret"

repos:
  org-a/repo-x:
    github_app: app-a
  org-b/repo-y:
    github_app: app-b

Validation rules

  • github: and apps: are mutually exclusive.
  • When apps: is set, every entry in repos: MUST set github_app: to one of the configured app names.
  • When apps: is NOT set, repos MUST NOT set github_app: (rejected explicitly — no silent ignore).
  • Each app entry MUST provide a non-empty app-id, private-key, and webhook-secret.
  • Empty apps: {} is rejected.

Resolver

func (c *ServerConfig) ResolveGitHubAppForRepo(repo string) (ResolvedGitHubApp, error)

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

  • Webhook dispatch (X-GitHub-Hook-Installation-Target-ID routing, per-App HMAC verification).
  • ClientSet and per-App installation token minting in pkg/github.
  • serve.go and recovery path wiring.
  • Per-App metrics/log labels.

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.


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.
Copilot AI review requested due to automatic review settings June 2, 2026 09:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of GitHubAppConfig, aliased to GitHubConfig) and RepoConfig.GitHubApp, with mutual-exclusivity and per-app/per-repo validation rules.
  • Introduce ResolvedGitHubApp and ServerConfig.ResolveGitHubAppForRepo, returning the legacy App under the synthetic name "default".
  • Document the new shape in docs/configuration.md and the Helm values.yaml example, 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.

Comment thread pkg/api/config.go
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.
aparajon
aparajon previously approved these changes Jun 2, 2026
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.
@Kiran01bm Kiran01bm marked this pull request as ready for review June 2, 2026 22:07
@Kiran01bm Kiran01bm requested a review from morgo as a code owner June 2, 2026 22:07
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread pkg/api/config.go Outdated
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.
@Kiran01bm Kiran01bm merged commit 1143bd5 into main Jun 2, 2026
24 checks passed
@Kiran01bm Kiran01bm deleted the kiran01bm/multi-github-app-config-schema branch June 2, 2026 22:24
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.

3 participants