-
Notifications
You must be signed in to change notification settings - Fork 11
Add support for S3 regions #962
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. 📝 WalkthroughWalkthroughAdded an optional Changes
Sequence Diagram(s)(omitted — changes are configuration additions, API client adjustments and a small helper; they do not introduce a new multi-component sequential flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Ruff (0.14.11)ami/main/migrations/0079_s3storagesource_region.py7-9: Mutable class attributes should be annotated with (RUF012) 11-22: 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 (7)
✏️ Tip: You can disable this entire section by setting 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.