Repository navigation
Conversation
…he cleanup timeout covers S3
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Test settings and Garage provisioning .env.example, internal/testdb/..., cmd/alphone/settings_test.go, test/s3/*, compose.yaml |
Adds required test-setting lookup, example S3 settings, and Garage configuration and provisioning for the test bucket and key. |
Bounded S3 abort cleanup internal/storage/storage.go, internal/storage/storage_internal_test.go, go.mod, .env.example, docs/src/content/docs/self-hosting/configuration.md |
The S3 client now uses the configured cleanup timeout for multipart-upload abort requests. Tests cover aborts after cancellation and when the store responds after the timeout. Cleanup descriptions now include S3 parts as well as PostgreSQL rows. |
S3 storage behavior coverage internal/storage/s3_test.go |
Adds tests for file operations, workspace isolation, oversized and failed uploads, multipart readback, and cleanup under both checksum settings. |
Local and CI test runs Makefile, .github/workflows/ci.yml, .golangci.yml, docs/src/content/docs/start/local-development.md, docs/src/content/docs/self-hosting/install.md |
Adds local and CI commands for S3-tagged tests, configures lint to use the s3 build tag, and documents test setup and the Garage checksum caveat. |
Priority:
Estimated code review effort: 3 (Moderate) | ~30 minutes
Change: Other
Possibly related PRs
- gopherium/AlphOne#301: Adds the S3 storage driver that this pull request tests against Garage.
- gopherium/AlphOne#282: Adds validation for settings in
.env.example; this pull request adds the S3 test settings to that validation allowlist.
Suggested reviewers: codescene-access
Merge Risk: 
Minimal · up to 0d840
-
No actionable issue remains from the reviewed S3 test and cleanup changes. Merge after normal checks.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Linked Issues check
-
Explanation
Issue
#302requires a Garage CI job, bucket and key setup, S3 storage tests, both checksum settings, multipart coverage, cleanup checks, and a job under 15 minutes. The reviewed changes add the pinned Garage job, create the bucket and key, add the requested save/open/delete, missing-file, over-cap, failed-upload, cleanup, checksum, and multipart tests, and set a 10-minute job timeout. The S3 cleanup fix supports the required cleanup behavior.
Full details: Out of Scope Changes check
-
Explanation
The changes stay related to
#302. The local Garage Compose profile, Makefile target, test helpers, configuration settings, documentation, and multipart cleanup fix support S3 test execution, diagnosis, or operation. No unrelated change is established by the reviewed summary.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 57.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 7 files. (10 skipped: 10 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
Gates Passed
4 Quality Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Code Review ✅ Approved🟡 Medium risk · S3 multipart aborts now outlive canceled upload contexts but are time-bounded, so a slow store could retain abandoned parts after timeout. Adds a VerificationRules No rules evaluated Functional validation Not enabledFor more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
[Medium impact] Safe to merge within the reviewed scope. The real-store integration suite and focused cancellation and timeout checks passed.
|
Closes #302.
What
s3, starts a pinned Garage store, makes a bucket and a key, and runs the storage tests built with the news3tag against it.ALPHONE_STORAGE_CLEANUP_TIMEOUT.make test-s3runs the same tests on a local Garage from a news3profile incompose.yaml.ALPHONE_STORAGE_S3_CHECKSUM=when_required.Why
Nothing tested the S3 driver against a real store, and the first run found the leftover parts.
Testing Instructions
Run the testing toolchain.
Summary by CodeRabbit