From ae09352e7531f10df6c93918cfd625cf6e05d759 Mon Sep 17 00:00:00 2001 From: jamy Date: Thu, 9 Jul 2026 21:21:28 +0000 Subject: [PATCH] docs(rfc): host in change ID URI format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Intent: - Embed the target host (domain/port) in every change ID URI so change identity is self-contained across multiple GitHub/GHES/Phabricator instances Changes: - Added RFC at doc/rfc/submitqueue/host-in-change-id.md covering the new URI format, migration path, stacked-change validation, and alternatives considered - Updated doc/rfc/index.md to link the new RFC ## Summary Adds an RFC documenting the design decision to embed the host (domain/port) in GitHub, GHES, and Phabricator change ID URIs. The old URI format encoded everything about a change except which server it lives on, making it impossible to distinguish changes across multiple instances of the same provider type (e.g. multiple GHES instances). The RFC covers the new URI format, migration strategy, stacked-change validation, and alternatives considered. ## Test Plan Existing CI/Build validation ## Revert Plan git revert ## Jira Issues ## API Changes No API changes ## Monitoring and Alerts Standard monitoring via existing dashboards 🤖 Generated by minion Co-Authored-By: minion-bot Co-Authored-By: jamy@uber.com Agent: claude-code | Task: 87c9d30e-c740-416a-9bce-201e86b2f14c View your task: https://minion.uberinternal.com/task/87c9d30e-c740-416a-9bce-201e86b2f14c Co-Authored-By: Claude Opus 4.6 --- doc/rfc/index.md | 1 + doc/rfc/submitqueue/host-in-change-id.md | 133 +++++++++++++++++++++++ 2 files changed, 134 insertions(+) create mode 100644 doc/rfc/submitqueue/host-in-change-id.md diff --git a/doc/rfc/index.md b/doc/rfc/index.md index 5baf9016..1ff5b0e0 100644 --- a/doc/rfc/index.md +++ b/doc/rfc/index.md @@ -14,6 +14,7 @@ Design documents and technical proposals, grouped by scope. Shared/cross-cutting - [Build Runner](submitqueue/build-runner.md) - Vendor-agnostic BuildRunner interface, provider-neutral BuildStatus lifecycle, and how the orchestrator wires it into the build stage - [Extension Contract](submitqueue/extension-contract.md) - When extensions take orchestrator identity (request/batch) and resolve granular content themselves vs. take controller-resolved data; revises the BuildRunner base/head contract - [Speculation](submitqueue/speculation.md) - Why SubmitQueue speculates, the path/tree model, and the two pluggable seams: speculation-tree enumeration and path selection +- [Host in Change ID URI](submitqueue/host-in-change-id.md) - Embed the target host in change ID URIs so change identity is self-contained across multiple GitHub/GHES/Phabricator instances ## Stovepipe diff --git a/doc/rfc/submitqueue/host-in-change-id.md b/doc/rfc/submitqueue/host-in-change-id.md new file mode 100644 index 00000000..b179dcd6 --- /dev/null +++ b/doc/rfc/submitqueue/host-in-change-id.md @@ -0,0 +1,133 @@ +# Host in Change ID URI + +Embed the target host (domain or domain:port) in every change ID URI so the identity of a change is self-contained — no out-of-band configuration required to know which server it lives on. + +## Problem + +Change ID URIs currently encode everything about a change except *where it lives*. A GitHub URI carries the scheme, org, repo, PR number, and head SHA, but not the hostname of the GitHub instance that hosts it: + +``` +github://uber/submitqueue/pull/123/c3a4d5e... ← no host +``` + +The host is resolved out-of-band: the `BaseURLTransport` in `platform/http` carries the API base URL, and the change provider factory is configured per queue with a hostname. This works when there is exactly one instance per scheme, but breaks as soon as a second instance of the same type appears — which is a realistic scenario: + +- **Multiple GHES instances.** Large organizations commonly run more than one GitHub Enterprise Server: one per business unit, one per compliance boundary, or a staging instance alongside production. Two PRs with the same org/repo/number on different GHES hosts are *different changes*, but their URIs are identical under the old format. +- **Phabricator migrations.** Teams migrating between Phabricator instances need both old and new revision URIs to coexist in the same queue without ambiguity. +- **Multi-tenant deployments.** A single SubmitQueue deployment serving queues across different GitHub instances must distinguish changes by host, not by separate deployments. + +Without the host in the URI, the system has no way to tell these apart at the identity level. The workaround — configuring a single host per scheme in the provider factory — is fragile and does not compose. + +The `git://` scheme already solved this: its URI includes a `Remote` field (the host) as part of the identity. GitHub and Phabricator URIs should follow the same pattern. + +## Decision + +Add the host as the first path segment after the scheme separator in all change ID URIs. + +### GitHub / GHE / GHES + +Old format: + +``` +{scheme}://{org}/{repo}/pull/{pr}/{sha} +``` + +New format: + +``` +{scheme}://{host}/{org}/{repo}/pull/{pr}/{sha} +``` + +Examples: + +``` +github://github.com/uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab +ghe://ghe.example.com/corp/service/pull/42/deadbeefdeadbeefdeadbeefdeadbeefdeadbeef +ghes://ghes.corp.net:8443/org/repo/pull/7/0123456789abcdef0123456789abcdef01234567 +``` + +The `ChangeID` struct gains a `Host` field. The parser requires at least 6 path segments (was 5): host, org, repo, `pull`, PR number, SHA. The host must be non-empty; ports are allowed (`host:port`). The `String()` serializer includes the host as the first segment after `://`. + +### Phabricator + +Old format: + +``` +phab://D{revision_id}/{diff_id} +``` + +New format: + +``` +phab://{host}/D{revision_id}/{diff_id} +``` + +Examples: + +``` +phab://phabricator.example.com/D12345/67890 +phab://phab.corp.net:8443/D42/99 +``` + +The `ChangeID` struct gains a `Host` field. The parser requires exactly 3 path segments (was 2): host, D-prefixed revision, diff ID. The host must be non-empty. + +### Validation + +Stacked changes (multiple URIs in a single `Change`) must share the same host. The `validateChangeConsistency` function in the GitHub change provider already checks scheme, org, and repo consistency; it now also checks host consistency. A mismatch produces a clear error: + +``` +stacked changes must be from same host: expected github.com, got ghes.corp.net for PR #42 +``` + +### Wire format + +The proto documentation in `api/base/change/proto/change.proto` is updated to reflect the new templates. The proto message itself (`Change`) is a simple `repeated string uris` — it carries opaque URI strings, so the wire format does not change. This is a URI-level identity change, not a proto schema change. + +## Scope + +### What changes + +- `platform/base/change/github/change_id.go` — `ChangeID` struct, `ParseChangeID`, `String()` +- `platform/base/change/phabricator/change_id.go` — `ChangeID` struct, `ParseChangeID`, `String()` +- `platform/base/change/change.go` — doc comments with URI templates +- `api/base/change/proto/change.proto` — doc comments with URI templates +- `submitqueue/extension/changeprovider/github/validate.go` — host consistency check +- All test files with hardcoded URI strings (~40 files) + +### What does not change + +- The `git://` scheme — it already has a `Remote` field serving the same purpose. +- The proto wire format — `Change.uris` is `repeated string`, unaffected. +- The `BaseURLTransport` — it continues to work as before; the host in the URI is for identity, not for HTTP routing. The transport's base URL is the API endpoint (e.g. `https://api.github.com`), which may differ from the host in the URI (e.g. `github.com`). These are complementary, not redundant. +- The routing provider (`routing/provider.go`) — it dispatches by scheme, not by host. A host-aware routing layer (dispatching to different API clients per host) is a future concern outside this RFC's scope. + +## Migration + +This is a breaking change to the URI format. All stored URIs (in queues, databases, logs) will use the old format until re-created. + +- **New URIs** are generated with the host by the gateway at submission time — the caller supplies the host as part of the URI. +- **Existing URIs** in flight will fail to parse with the new parser. Since SubmitQueue processes requests to completion quickly (minutes, not days), a deploy that drains in-flight requests before upgrading the parser is sufficient. No backfill migration is needed. +- **Persisted URIs** in change records or historical data are immutable. If historical queries must work across the format boundary, the reader should fall back to the old parser on failure. This is a bounded compatibility window, not a permanent dual-parse requirement. + +## Alternatives considered + +### Host as a query parameter + +``` +github://uber/repo/pull/123/sha?host=ghes.corp.net +``` + +This keeps the path structure unchanged but makes the host optional and easy to omit. The host is part of the change's identity — it belongs in the primary path, not in metadata. + +### Separate scheme per host + +``` +ghes-corp://org/repo/pull/123/sha +ghes-staging://org/repo/pull/123/sha +``` + +This encodes the host in the scheme, but schemes are a finite enumerated set in the routing provider. Adding a scheme per host couples deployment topology to the URI grammar and requires code changes for every new instance. + +### Keep host out-of-band, add a host field to the ChangeID struct only + +Callers would populate `ChangeID.Host` from configuration rather than parsing it from the URI. This splits identity between the URI (partial) and the struct (full), making the URI non-self-describing. Any system that only sees the URI string — logs, dashboards, queue payloads — cannot determine which host a change belongs to.