Skip to content

fix(s3): default to s3v4 signature when no config is provided#50

Merged
Andarius merged 1 commit intomasterfrom
fix/s3-niquests-version
Mar 12, 2026
Merged

fix(s3): default to s3v4 signature when no config is provided#50
Andarius merged 1 commit intomasterfrom
fix/s3-niquests-version

Conversation

@Andarius
Copy link
Contributor

@Andarius Andarius commented Mar 12, 2026

  • Default s3_config to Config(signature_version="s3v4") in S3Session when none is supplied
  • Add AGENTS.md to .gitignore

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced S3 client configuration to properly apply default settings when no custom configuration is specified.
  • Chores

    • Updated build artifact ignore list.

- Default `s3_config` to `Config(signature_version="s3v4")` in `S3Session` when none is supplied
- Add `AGENTS.md` to `.gitignore`
@Andarius Andarius self-assigned this Mar 12, 2026
@Andarius Andarius marked this pull request as ready for review March 12, 2026 09:22
@coderabbitai
Copy link

coderabbitai bot commented Mar 12, 2026

Walkthrough

Two files modified: .gitignore now excludes AGENTS.md, and tracktolib/s3/niquests.py implements a default S3 signature configuration (s3v4) in S3Session.__post_init__ when no explicit s3_config is provided.

Changes

Cohort / File(s) Summary
Configuration/Ignore files
.gitignore
Added AGENTS.md to the ignore list.
S3 Session initialization
tracktolib/s3/niquests.py
Modified S3Session.__post_init__ to ensure a default Config with signature_version="s3v4" is used when no s3_config is supplied, improving initialization robustness without altering the public API.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the primary change: defaulting to s3v4 signature in S3Session when no config is provided, which matches the main modification in tracktolib/s3/niquests.py.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/s3-niquests-version

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tracktolib/s3/niquests.py`:
- Around line 218-225: The code currently constructs a fallback
Config(signature_version="s3v4") only for passing into
self._botocore_session.create_client, leaving self.s3_config as None; update the
logic so that if self.s3_config is None you assign that fallback Config to
self.s3_config before calling self._botocore_session.create_client (i.e., set
self.s3_config = Config(signature_version="s3v4") and then call create_client
with config=self.s3_config) while keeping the existing endpoint_url and region
arguments (self.endpoint_url, self.region).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a01a7533-f1e8-4f2d-8a66-5c08ec2a766c

📥 Commits

Reviewing files that changed from the base of the PR and between e795d1f and c52035e.

📒 Files selected for processing (2)
  • .gitignore
  • tracktolib/s3/niquests.py

Comment on lines +218 to +225
config = self.s3_config
if config is None:
config = Config(signature_version="s3v4")
self.s3_client = self._botocore_session.create_client(
"s3",
endpoint_url=self.endpoint_url,
region_name=self.region,
config=self.s3_config,
config=config,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Persist the fallback config on self.s3_config.

Lines 218-225 only pass the derived Config(signature_version="s3v4") into create_client, so session.s3_config still ends up as None. That leaves the public session state inconsistent with the effective client config and doesn’t fully implement the PR’s stated “default s3_config” behavior.

Suggested fix
         if self.s3_client is None:
             self._botocore_session = botocore.session.Session()
             if self.access_key is not None and self.secret_key is not None:
                 self._botocore_session.set_credentials(self.access_key, self.secret_key)
-            config = self.s3_config
-            if config is None:
-                config = Config(signature_version="s3v4")
+            if self.s3_config is None:
+                self.s3_config = Config(signature_version="s3v4")
             self.s3_client = self._botocore_session.create_client(
                 "s3",
                 endpoint_url=self.endpoint_url,
                 region_name=self.region,
-                config=config,
+                config=self.s3_config,
             )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
config = self.s3_config
if config is None:
config = Config(signature_version="s3v4")
self.s3_client = self._botocore_session.create_client(
"s3",
endpoint_url=self.endpoint_url,
region_name=self.region,
config=self.s3_config,
config=config,
if self.s3_config is None:
self.s3_config = Config(signature_version="s3v4")
self.s3_client = self._botocore_session.create_client(
"s3",
endpoint_url=self.endpoint_url,
region_name=self.region,
config=self.s3_config,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tracktolib/s3/niquests.py` around lines 218 - 225, The code currently
constructs a fallback Config(signature_version="s3v4") only for passing into
self._botocore_session.create_client, leaving self.s3_config as None; update the
logic so that if self.s3_config is None you assign that fallback Config to
self.s3_config before calling self._botocore_session.create_client (i.e., set
self.s3_config = Config(signature_version="s3v4") and then call create_client
with config=self.s3_config) while keeping the existing endpoint_url and region
arguments (self.endpoint_url, self.region).

@Andarius Andarius merged commit 1982b25 into master Mar 12, 2026
4 checks passed
@Andarius Andarius deleted the fix/s3-niquests-version branch March 12, 2026 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant