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.
|
|
||
| import boto3 | ||
| import boto3.resources.base | ||
| import boto3.session |
There was a problem hiding this comment.
The import boto3.session is not used in this file. The code uses boto3.Session which is available through the main boto3 import. This import should be removed.
| import boto3.session |
| endpoint_url=config.endpoint_url, | ||
| # api_version="s3v4", | ||
| ) | ||
| s3 = typing.cast(S3ServiceResource, s3) |
There was a problem hiding this comment.
This type cast is unnecessary. The session.resource() method already returns the correct type. Remove this line to simplify the code.
| s3 = typing.cast(S3ServiceResource, s3) |
There was a problem hiding this comment.
I does not detect the correct type without this
mihow
force-pushed
the
fix/add-s3-region
branch
from
aff251a to
15a4edd
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.14.7)ami/main/migrations/0079_s3storagesource_region.py7-9: Mutable class attributes should be annotated with (RUF012) 11-17: Mutable class attributes should be annotated with (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)
🔇 Additional comments (1)
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.
|
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 theS3StorageSourcemodelHow to Test the Changes
.
Screenshots
If applicable, add screenshots to help explain this PR (ex. Before and after for UI changes).
Deployment Notes
.
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.