feat: multi-installation GitHub App support + images search#346
feat: multi-installation GitHub App support + images search#346bencered wants to merge 1 commit intoColeMurray:mainfrom
Conversation
Greptile SummaryThis PR adds multi-installation GitHub App support by introducing
|
packages/control-plane/src/source-control/providers/github-provider.ts
Outdated
Show resolved
Hide resolved
670e4ab to
085082a
Compare
085082a to
d5228d2
Compare
- getAllGitHubAppConfigs() parses comma-separated GITHUB_APP_INSTALLATION_ID - listRepositories() merges and deduplicates repos across all installations - checkRepositoryAccess() throws when any installation errors (stricter, safer) - listBranches() and generatePushAuth() iterate all installations to find the owning token - Images settings: search input to filter repositories (aria-label for a11y) - deploy-cf.sh: reads secrets from env vars (no hardcoded values) - 45 tests covering multi-installation fallback, error surfacing, deduplication
d5228d2 to
a8e354f
Compare
|
what's the use-case? |
Allows people who work across multiple GitHub organisations (e.g. agencies, contractors) to use a single OpenInspect instance across all of them. Means that they wouldn't have to set up a fresh instance for each org. |
Multi-installation gap analysisReviewed the full codebase to understand the blast radius. The PR adds multi-installation parsing and routing on the TypeScript control plane, but several other layers still assume a single installation ID. Here's the complete picture. 1.
|
| Layer | Issue | Severity |
|---|---|---|
pull-request-service.ts |
generatePushAuth() called without repo context |
Bug — PR pushes to secondary-installation repos fail |
| Python/Modal (all token sites) | Comma-separated ID passed verbatim to GitHub API URL | Breaking — malformed URL, no tokens generated for any repo |
github-bot/handlers.ts |
Reads env var directly, not via parser | Breaking — bot webhook handling fails entirely |
| Terraform | Single string var, no per-service splitting | Config gap |
| Image builder cron | No per-repo installation routing | Feature gap |
The TypeScript control plane changes are well-structured. The core issue is that setting GITHUB_APP_INSTALLATION_ID to comma-separated values will break the Python/Modal and github-bot layers before multi-installation routing can even be exercised. These layers need corresponding changes to parse (or receive) the correct installation ID per-repo.
|
based on the gap analysis, i'm wondering if it makes sense to support this. While I do see the convenience of having multiple appIds, it adds a bit of oddities where pretty much any call to interact with GH will be required to loop multiple appIds, all but one of which will fail. Open to suggestions, but my thoughts are it is probably better to deploy multiple instances of the control-plane. |
Changes
Multi-installation GitHub App support
getAllGitHubAppConfigs()parses comma-separatedGITHUB_APP_INSTALLATION_IDenv varlistRepositories()merges and deduplicates repos across all installationscheckRepositoryAccess()falls back through installations; throws when ALL fail (vs silent null)Images settings search
aria-labelfor accessibilityTests
Config
terraform.tfvarsupdated with comma-separated installation IDs (gitignored)