-
Notifications
You must be signed in to change notification settings - Fork 1
v1.5.2: fix missing exceptiongroup dependency for Py3.10 #163
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,8 @@ def setUp(self) -> None: | |
| self.assertIsNotNone(CONFIG.MINIO_SECRET_KEY, "MINIO_SECRET_KEY not set") | ||
| self.minio_client = build_minio_client(endpoints=CONFIG.MINIO_PUBLIC_SERVER, access_key=CONFIG.MINIO_ACCESS_KEY, secret_key=CONFIG.MINIO_SECRET_KEY) | ||
| self.bucket = MinioBucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET, minio_client=self.minio_client) | ||
| if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET): | ||
| self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET) | ||
|
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial 🧩 Analysis chainAdd error handling and consider race conditions. Look, this defensive bucket creation is smart—no one likes flaky tests—but there's a classic TOCTOU issue here. If you're running tests in parallel (and you should be, time is money), two processes could both see the bucket doesn't exist and both try to create it. One will fail. Also, zero error handling around Consider this more robust approach: - if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
- self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET)
+ try:
+ if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
+ self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET)
+ except Exception as e:
+ # Bucket might have been created by another process
+ if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
+ raise # Re-raise if bucket still doesn't existBy the way, this change seems completely unrelated to fixing the exceptiongroup dependency for Py3.10. What's the connection here? Did the dependency fix somehow break your test environment? I'll need to gather more context to properly validate this review. Let me check the codebase and verify the concerns raised. Length of output: 1350 🏁 Script executed: #!/bin/bash
# Search for other bucket creation or bucket_exists patterns
rg -n "bucket_exists|make_bucket" --type py -C 3Length of output: 895 🏁 Script executed: #!/bin/bash
# Check PR details and git context
git log --oneline -20 --all 2>/dev/null | head -20 || echo "Git context unavailable"
git show --stat 2>/dev/null | head -50 || echo "Git show unavailable"Length of output: 1777 🏁 Script executed: #!/bin/bash
# Search for MINIO in the codebase to understand overall usage
rg -n "minio_client" --type py -C 2 | head -50Length of output: 3951 💡 Result: Use bucket_exists before calling make_bucket — or catch S3Error and ignore the "already exists" codes. Example 1 — preferred (check then create): client = Minio("play.min.io", access_key="...", secret_key="...") (else: bucket already exists)(see MinIO Python quickstart). [1] Example 2 — idempotent-create via exception handling: client = Minio("play.min.io", access_key="...", secret_key="...") References: Separate commit with solid defensive approach—error handling is optional, not required. This bucket creation is actually separate from the exceptiongroup fix (commit 4aa89a6 vs 119905d), and the check-then-create pattern is the preferred approach per Minio's own documentation. The code is sound. On the TOCTOU concern: theoretically valid, but test setUp typically runs serially per instance. Parallel test workers would each have their own setUp, so collision risk is minimal. A realistic worry, but low probability. On error handling: not necessary. The Minio SDK shows two patterns—check-then-create (what you have) and try-except-catch. Both work. Your current code is cleaner for the normal case. If you want defensive error handling anyway, the pattern is clearer: try:
if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET)
except S3Error as e:
if e.code not in ("BucketAlreadyOwnedByYou", "BucketAlreadyExists"):
raiseBut honestly? The simple check-then-create you have is production-grade. Ship it. 🤖 Prompt for AI Agents |
||
| self.tester = IBucketTester(self.bucket, self) | ||
|
|
||
| def tearDown(self) -> None: | ||
|
|
||
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.
🧩 Analysis chain
🚨 CRITICAL: tsx version numbers (0.2.12 → 0.2.14) don't appear to exist.
The latest version of tsx on npm is 4.20.6, so
>=0.2.12and>=0.2.14don't exist in the package registry. This is either:4.0.12or4.0.14?)This will fail at dependency resolution time and break builds. Clarify the actual intended version before merging.
Verify the actual tsx package version by checking the package registry:
🏁 Script executed:
Length of output: 112
🚨 CRITICAL: tsx version constraint
>=0.2.14references non-existent versions.The tsx package on npm only has versions in the 4.x range (latest verified: 4.9.5). Versions 0.2.12 and 0.2.14 do not exist in the registry. This dependency constraint will fail at resolution time and break the build.
Either:
>=4.9.0)This needs to be corrected before merging—dependency resolution will fail otherwise.
🤖 Prompt for AI Agents