feat: add S3-compatible endpoint support for custom object stores#895
feat: add S3-compatible endpoint support for custom object stores#895
Conversation
Thread optional `endpoint` and `forcePathStyle` fields through the S3 datastore stack so users can target S3-compatible providers like DigitalOcean Spaces. Both fields are conditionally spread into the AWS SDK client constructor, preserving existing behavior when omitted. Adds `--endpoint` and `--force-path-style` CLI flags to `swamp datastore setup s3` and supports the fields in `.swamp.yaml`.
There was a problem hiding this comment.
Code Review
Clean, well-structured PR that threads endpoint and forcePathStyle through the full S3 stack consistently. No blocking issues found.
Blocking Issues
None.
Suggestions
-
S3DatastoreVerifierconstructor — consider a config object: The constructor now takes 5 positional optional parameters (bucket, prefix?, region?, endpoint?, forcePathStyle?), which is fragile — callers must remember the order and can't skip middle params. Consider accepting anS3ClientConfig(or a subset) instead, matching howS3Clientitself is constructed. This is a pre-existing pattern issue that this PR extends, so not blocking. -
DatastoreSetupDepsinterface positional params: TheverifyS3Datastore,checkS3DatastoreExists, andpushToS3function signatures in the deps interface similarly grow with positional optionals. Passing an options object would make these more maintainable and testable. Again, pre-existing pattern — not blocking for this PR.
Overall: DDD layering is correct (infrastructure config in infrastructure, domain types in domain), the libswamp import boundary is respected, tests cover the new fields, and the conditional spread pattern in S3Client constructor correctly avoids passing undefined to the AWS SDK.
LGTM ✅
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
-
src/libswamp/datastores/setup.ts:311-318— "already exists" YAML hint omitsendpointandforcePathStyle.
When an S3 location already has a datastore, the error message prints a YAML snippet for the user to copy into.swamp.yaml. That snippet includesbucket,prefix, andregion, but notendpointorforcePathStyle. A user targeting DigitalOcean Spaces who hits this path would get an incomplete config suggestion that points at AWS S3 instead of their custom endpoint.Breaking example:
swamp datastore setup s3 --bucket my-space --endpoint https://nyc3.digitaloceanspaces.com --force-path-styleon a location that already has data → the YAML hint tells the user to configure without endpoint/forcePathStyle → they paste it →swamp datastore sync --pullfails because it tries to reach AWS.Suggested fix: Add the two fields to the YAML hint:
(input.endpoint ? ` endpoint: ${input.endpoint}\n` : "") + (input.forcePathStyle ? ` forcePathStyle: true\n` : "") +
Low
src/infrastructure/persistence/s3_datastore_verifier.ts:36-41— Five positional optional parameters.
The constructor signature(bucket, prefix?, region?, endpoint?, forcePathStyle?)is fragile — a caller could easily swap argument order (e.g., pass endpoint as region). An options object would be safer, matching the pattern used byS3Client. Not blocking since all current call sites are correct, but worth noting for future maintainability.
Verdict
PASS — Straightforward feature addition with correct conditional-spread logic, proper null handling (!= null for forcePathStyle), and no behavioral regressions. The medium finding is a UX papercut in an uncommon error path, not a correctness issue.
There was a problem hiding this comment.
CLI UX Review
Blocking
None.
Suggestions
-
Column alignment in
src/presentation/renderers/datastore_status.ts: All other labels are padded to 9 characters before the value (Region:,Exclude:,Health:), but the new entry usesEndpoint:(10 chars), shifting the value one column to the right. ConsiderEndpoint:with two trailing spaces to match the pattern, or accept this as a minor cosmetic inconsistency. -
forcePathStylenot surfaced in status output:--force-path-styleis accepted at setup time and persisted to the marker, but it is not included inDatastoreStatusData, soswamp datastore status(both log and JSON mode) gives no indication whether path-style addressing is active. Users targeting S3-compatible stores may want to verify their config is correct. Worth addingforcePathStyle?: booleantoDatastoreStatusDataand rendering it alongsideEndpoint.
Verdict
PASS — new --endpoint and --force-path-style flags are well-named, clearly described (with a helpful example URL), and endpoint is correctly surfaced in both log and JSON status output. No blocking UX issues.
Summary
endpointandforcePathStylefields through the S3 datastore stack so users can target S3-compatible providers like DigitalOcean Spacesundefined) into the AWS SDK client constructor, preserving existing behavior when omitted--endpointand--force-path-styleCLI flags toswamp datastore setup s3swamp datastore statusoutputTest Plan
endpointfielddeno check,deno lint,deno fmt,deno run compileall pass🤖 Generated with Claude Code