Add support for S3 regions #962
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for S3 regions to enable users in different AWS regions (particularly Europe) to properly configure their S3 storage backends. Previously, the region parameter was omitted for simplicity as university clouds didn't require it and the default region worked for existing AWS users.
- Adds optional
regionfield to theS3StorageSourcemodel and corresponding configuration - Updates S3 client initialization to properly handle region configuration
- Fixes existing type annotation issues in S3 utility functions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ami/main/models.py | Adds nullable region field to S3StorageSource model and includes it in the config property |
| ami/main/migrations/0075_s3storagesource_region.py | Database migration to add the region field to existing S3 storage sources |
| ami/utils/s3.py | Updates S3 client/session initialization to use region and fixes type annotations |
| ami/tests/fixtures/storage.py | Updates test fixture to include region parameter |
| config/settings/base.py | Adds S3_TEST_REGION environment variable configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mihow
force-pushed
the
fix/add-s3-region
branch
from
aff251a to
15a4edd
Compare
📝 WalkthroughWalkthroughAdds optional AWS region support across S3 storage: new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ami/tests/fixtures/storage.py (1)
25-39: Consider passingregiontoS3StorageSourcedefaults.The
S3_TEST_CONFIGnow includesregion, butcreate_storage_sourcedoesn't pass it to theS3StorageSource.objects.get_or_createdefaults. This could lead to inconsistency where the test config has a region but the created storage source doesn't.defaults=dict( bucket=S3_TEST_CONFIG.bucket_name, prefix=prefix, endpoint_url=S3_TEST_CONFIG.endpoint_url, access_key=S3_TEST_CONFIG.access_key_id, secret_key=S3_TEST_CONFIG.secret_access_key, public_base_url=S3_TEST_CONFIG.public_base_url, + region=S3_TEST_CONFIG.region, ),ami/utils/s3.py (1)
131-139: Consider passingregion_nameandconfigto the resource for consistency.Unlike
get_s3_client, this function doesn't explicitly passregion_nameor thebotocore.config.Configwith signature version. While the session inherits the region fromget_session(), explicitly passing these parameters would ensure consistent behavior and make the code more explicit.def get_resource(config: S3Config) -> S3ServiceResource: session = get_session(config) + boto_config = botocore.config.Config(signature_version="s3v4") s3 = session.resource( "s3", endpoint_url=config.endpoint_url, - # api_version="s3v4", + region_name=config.region, + config=boto_config, ) s3 = typing.cast(S3ServiceResource, s3) return s3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ami/main/migrations/0075_s3storagesource_region.py(1 hunks)ami/main/models.py(2 hunks)ami/tests/fixtures/storage.py(1 hunks)ami/utils/s3.py(6 hunks)config/settings/base.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
ami/main/migrations/0075_s3storagesource_region.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (9)
config/settings/base.py (1)
448-452: LGTM!The new
S3_TEST_REGIONsetting follows the established pattern for S3 test configuration and appropriately defaults toNonefor backward compatibility with existing setups that don't require region specification.ami/main/models.py (1)
1389-1423: LGTM!The
regionfield is properly added toS3StorageSourceand correctly propagated toS3Configin theconfigproperty. The nullable design appropriately maintains backward compatibility for existing storage sources and S3-compatible backends that don't require region specification.ami/main/migrations/0075_s3storagesource_region.py (1)
1-17: LGTM!Standard Django migration for adding the nullable
regionfield. The static analysis hints aboutClassVarannotations are false positives — Django migrations conventionally definedependenciesandoperationsas class attributes without type annotations.ami/utils/s3.py (6)
13-13: Theboto3.sessionimport is used.Contrary to the previous review comment, this import is used for the return type annotation on line 94:
def get_session(config: S3Config) -> boto3.session.Session. The import should be retained.
33-41: LGTM!The
regionfield is properly added to theS3Configdataclass with an appropriateNonedefault for backward compatibility.
94-100: LGTM!The session now correctly receives
region_namefrom the config, ensuring region-aware S3 operations.
103-128: LGTM! The client now consistently uses Signature Version 4 and the region parameter.Minor note: The credentials are passed to both the session (via
get_session) and the client. This redundancy is harmless—the client parameters take precedence—but could be simplified in a future refactor.
599-601: LGTM!Good improvement extracting
Bodyto a separate variable. TheStreamingBodyis file-like but type checkers don't recognize it, so thetype: ignorecomment is appropriate.
684-695: LGTM!The test function is properly updated with the
region=Noneparameter to match the updatedS3Configsignature.
✅ Deploy Preview for antenna-ssec canceled.
|
Ensure consistency between S3_TEST_CONFIG and created S3StorageSource instances by passing the region parameter to the defaults dict. This prevents potential inconsistencies where the test config has a region but the created storage source doesn't. Co-Authored-By: Claude <noreply@anthropic.com>
Update get_resource() to explicitly pass region_name and botocore Config with signature version 4, matching the pattern used in get_s3_client(). This ensures consistent S3 behavior across both client and resource interfaces. Co-Authored-By: Claude <noreply@anthropic.com>
AWS requires CreateBucketConfiguration with LocationConstraint for buckets created outside us-east-1. Updated create_bucket() to handle this properly while maintaining compatibility with Swift/MinIO. Added docstring noting this function is primarily used for testing, as production users create their own buckets. Co-Authored-By: Claude <noreply@anthropic.com>
Add guidance that Swift/MinIO users should leave the region field blank, while AWS users should specify their region (e.g., 'us-east-1', 'eu-west-1'). Co-Authored-By: Claude <noreply@anthropic.com>
mihow
left a comment
mihow
left a comment
There was a problem hiding this comment.
Tested on staging with existing storage configurations and with a new project with images in a European S3 region
|
@coderabbitai add a ticket for adding the region field to the Storage Configuration form modal & backend API (serializer) so that users can configure a region from the UI. Preferably the updated form can introduce a collapsible "Advanced" settings area because we have quite a few fields now. Also! Please update the HTML for the images in the description of this PR - the images should use their natural width rather than stretching them to full width (just don't specify a width). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 98
GitHub Issue to CreateTitle: Add region field to Storage Configuration UI with advanced settings section Body: ## Description
Add support for configuring the S3 region field through the Storage Configuration form modal in the UI.
## Requirements
- [ ] Update the Storage Configuration serializer (backend API) to accept and validate the `region` field
- [ ] Add `region` field to the Storage Configuration form modal
- [ ] Introduce a collapsible "Advanced Settings" section in the form to better organize the growing number of fields
- [ ] Move appropriate fields (including the new `region` field) into the Advanced Settings section
- [ ] Ensure the region field includes appropriate help text (e.g., "AWS region for the S3 bucket. Leave blank for Swift/MinIO storage.")
- [ ] Add client-side validation if needed
- [ ] Update any relevant documentation
## Context
The `region` field was added to the `S3StorageSource` model in PR `#962` but is currently only configurable through the Django admin interface or database directly. Users should be able to configure this through the main UI.
## Related
- PR: https://github.com/RolnickLab/antenna/pull/962
- Comment: https://github.com/RolnickLab/antenna/pull/962#discussion_r[comment_id]
## Assignee
`@mihow`PR Description Image FixFor the PR description, please remove the <img width="100%" src="...">to: <img src="...">This will allow images to display at their natural width instead of stretching to full width. Would you like me to try creating the issue through a different method, or can you create it manually with the content above? |
Summary
Previously we omitted the
regionparameter for simplicity because the S3 implementations in the university compute clouds do not use regions and the default region worked for existing AWS S3 users. Now we have some users in Europe that need to specify the S3 region in order for the storage backend to sync, to display source images in the UI, and to processes source images.List of Changes
regionfield to theS3StorageSourcemodelFollow-up: add a region field to the storage source form field in the UI & in the API serializer.
How to Test the Changes
Screenshots
Before, when adding an S3 bucket in a European region
After
Deployment Notes
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.