Parse any replica URL scheme, not just s3:// - #26
Open
distronode-com wants to merge 1 commit into
Open
Conversation
The Storage settings page trimmed a literal "s3://" off LITESTREAM_REPLICA_URL and then cut at the first '/'. For every other scheme Litestream supports, nothing was trimmed and the first '/' found was the one inside "://", so the bucket name came out as the scheme: gcs://my-bucket/calnode -> "gcs:" abs://my-container/calnode -> "abs:" file:///var/lib/calnode -> "file:" s3://my-bucket/calnode -> "my-bucket" (the only case that worked) Measured against the old expression rather than inferred. Hit on a live GCS deployment, where the admin page reported the backup bucket as "gcs:". Replaced with replicaBucket(), which strips any leading <scheme>:// and takes the authority up to the first path separator. A file:// replica and a bare path yield "" — a file replica has no bucket, and saying so beats inventing one. The empty string stays empty, because backups_configured is computed from the raw value and "" here has to keep meaning "nothing to show". recordingStorage() in livekit_recording.go carried a byte-identical copy of the same parse and now shares the helper. There the consequence was worse than a display bug: with credentials present, a gcs:// or abs:// replica would have sent meeting recordings to a bucket literally named "gcs:". Behaviour for the already-correct cases is unchanged, including the bucket == "" guard that keeps recording storage reporting not-ready for a file replica. Tests: a table over every scheme (s3, gcs, abs, file, bare path, empty, scheme only, unknown scheme) against the parser, plus a GET /v1/settings/storage test that pins backups_bucket and backups_configured at the boundary the bug was reported from. Verified: gofmt -l . empty, go vet ./... clean, go test ./... exit 0 (26 packages).
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes in this run: full diff of PR #26 (scheme-agnostic replica URL bucket parse for settings display and recording storage).
replicaBuckethelper — Strips any leading<scheme>://then takes the authority up to the first/, sogcs:///abs://no longer report the scheme as the bucket.- Shared call sites —
GetStorageSettingsandrecordingStorage()both use the helper;file:/// empty still yield no bucket and keep the existing empty-bucket guard. - Tests — Table unit coverage plus a
GET /v1/settings/storageboundary test that would have failed on the oldgcs:bug. Local run of those tests passed.
Grok | 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

A
gcs://replica URL displays asgcs:in Settings → StorageGetStorageSettingstrimmed a literal"s3://"offLITESTREAM_REPLICA_URLand then cutat the first
/. For any other scheme Litestream supports, nothing was trimmed and thefirst
/found was the one inside://, so the bucket name came out as the scheme.Measured against the old expression rather than inferred:
LITESTREAM_REPLICA_URLbackups_buckets3://my-bucket/calnodemy-bucket(the only case that worked)gcs://my-bucket/calnodegcs:abs://my-container/calnodeabs:file:///var/lib/calnodefile:We hit this on a live GCS deployment: the admin page reported the backup bucket as
gcs:.The fix
A small
replicaBucket()helper strips any leading<scheme>://and takes the authorityup to the first path separator. No new dependency, no
net/urlround-trip.s3://,gcs://,abs://→ the bucket or container name.file:///var/lib/calnodeand a bare path →"". A file replica has no bucket, andsaying so beats inventing one; the page already renders an em dash for an empty value.
""stays"", becausebackups_configuredis computed from the raw value and""here has to keep meaning "nothing to show".
One thing beyond the reported bug
recordingStorage()inlivekit_recording.gocarried a byte-identical copy of the sameparse and now shares the helper. There the consequence was worse than a display bug: with
credentials present, a
gcs://orabs://replica would have sent meeting recordings to abucket literally named
gcs:. Behaviour for the already-correct cases is unchanged,including the
bucket == ""guard that keeps recording storage reporting not-ready for afile://replica. Happy to split this into its own commit if you would rather review itseparately.
Testing
go test ./...green (26 packages);gofmt -l .empty;go vet ./...clean.TestReplicaBucket— a table over s3, gcs, abs, file, a bare path, the empty string, ascheme with no bucket, and an unknown scheme.
TestGetStorageSettings_reportsTheBucketForEveryReplicaScheme— the same cases throughGET /v1/settings/storage, pinningbackups_bucketandbackups_configuredat theboundary the bug was reported from. There were no tests for this endpoint before.