fix(storage): support AWS_S3_ADDRESSING_STYLE env var for S3 virtual/path addressing#9062
fix(storage): support AWS_S3_ADDRESSING_STYLE env var for S3 virtual/path addressing#9062mathcoder23 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughS3Storage now imports botocore.config.Config and reads AWS_S3_ADDRESSING_STYLE during initialization to build a shared boto_config that sets signature_version and optionally configures s3.addressing_style. Both MinIO and non-MinIO boto3 clients use this unified config instead of ad-hoc configuration. ChangesS3 Addressing Style Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
mathcoder23
force-pushed
the
cody/fix-s3-virtual-address
branch
from
05d338f to
c279c31
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/settings/test_storage.py (1)
276-277: ⚡ Quick winAvoid asserting on botocore private internals in tests.
_user_provided_optionsis a private attribute (prefixed with underscore) and is not part of botocore's documented public API, making this test brittle across botocore versions. Use the publicConfig.s3dictionary instead—for example, assert thatconfig.s3 is Nonewhen addressing_style is 'auto' (the default behavior).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/unit/settings/test_storage.py` around lines 276 - 277, The test currently inspects botocore's private attribute _user_provided_options (call_kwargs["config"]._user_provided_options) which is brittle; replace that assertion with the public API by checking call_kwargs["config"].s3 is None when addressing_style is 'auto' (or not set). Update the assertion in the test_storage test that verifies S3 Config to use call_kwargs["config"].s3 is None instead of inspecting the private _user_provided_options attribute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/settings/storage.py`:
- Around line 44-53: The code reads AWS_S3_ADDRESSING_STYLE but doesn't
normalize/validate it, so values like " Virtual " or typos silently fall back to
auto; update the retrieval of AWS_S3_ADDRESSING_STYLE to use .strip().lower(),
validate against the allowed set ("virtual","path","auto"), and if the value is
invalid log or raise a clear error before constructing boto_config; then use the
normalized addressing_style when building the Config (see addressing_style,
AWS_S3_ADDRESSING_STYLE, boto_config, Config).
---
Nitpick comments:
In `@apps/api/plane/tests/unit/settings/test_storage.py`:
- Around line 276-277: The test currently inspects botocore's private attribute
_user_provided_options (call_kwargs["config"]._user_provided_options) which is
brittle; replace that assertion with the public API by checking
call_kwargs["config"].s3 is None when addressing_style is 'auto' (or not set).
Update the assertion in the test_storage test that verifies S3 Config to use
call_kwargs["config"].s3 is None instead of inspecting the private
_user_provided_options attribute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e530e5da-be20-4937-bd78-b80f264614c8
📒 Files selected for processing (2)
apps/api/plane/settings/storage.pyapps/api/plane/tests/unit/settings/test_storage.py
| addressing_style = os.environ.get("AWS_S3_ADDRESSING_STYLE", "auto").lower() | ||
|
|
||
| # Create boto3 Config with addressing style if explicitly set | ||
| if addressing_style in ("virtual", "path"): | ||
| boto_config = Config( | ||
| signature_version="s3v4", | ||
| s3={"addressing_style": addressing_style}, | ||
| ) | ||
| else: | ||
| boto_config = Config(signature_version="s3v4") |
There was a problem hiding this comment.
Normalize and validate AWS_S3_ADDRESSING_STYLE to avoid silent misconfiguration.
A value like " Virtual " or typo currently falls through to auto without any signal, which can hide config mistakes and make debugging provider-specific failures harder. Normalize with .strip().lower() and explicitly guard allowed values.
💡 Proposed fix
- addressing_style = os.environ.get("AWS_S3_ADDRESSING_STYLE", "auto").lower()
+ addressing_style = os.environ.get("AWS_S3_ADDRESSING_STYLE", "auto").strip().lower()
+ if addressing_style not in {"auto", "virtual", "path"}:
+ raise ValueError(
+ "AWS_S3_ADDRESSING_STYLE must be one of: auto, virtual, path"
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/plane/settings/storage.py` around lines 44 - 53, The code reads
AWS_S3_ADDRESSING_STYLE but doesn't normalize/validate it, so values like "
Virtual " or typos silently fall back to auto; update the retrieval of
AWS_S3_ADDRESSING_STYLE to use .strip().lower(), validate against the allowed
set ("virtual","path","auto"), and if the value is invalid log or raise a clear
error before constructing boto_config; then use the normalized addressing_style
when building the Config (see addressing_style, AWS_S3_ADDRESSING_STYLE,
boto_config, Config).
|
|
|
|
2 participants
Description
Type of Change
Changes made:
Usage:
Notes:
Screenshots and Media (if applicable)
The issue with image uploads occurs when using S3 with a cloud storage provider that does not support path-style access mode.
Test Scenarios
References
Summary by CodeRabbit
New Features
Tests