Deployment 0.3.69#317
Open
konstantinoscs wants to merge 4 commits into
Open
Conversation
It's not necessary, and it might be causing the entire workflow to fail
📝 WalkthroughWalkthroughUpdates CI workflows (build-binaries matrix restructuring to ChangesCI/Build Workflow Updates
Estimated code review effort: 2 (Simple) | ~10 minutes Catalog Credential Vending
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
tests/tower/test_tables.py (1)
174-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the
describe_catalogexception/failure path.
describe_catalog_api_synchere is only stubbed to returnNoneor aDescribeCatalogResponse; there's no test where it raises (simulating a network error or timeout), which is exactly theexcept Exceptionbranch in_describe_tower_catalog_type(src/tower/_storage.py, lines 142-156). Since this PR's stated goal is fixing credential-vending decisions for BYO/S3 tables, covering the "Tower API call fails" fallback-to-PyIceberg-config path would meaningfully harden this critical path.def test_string_catalog_falls_back_when_describe_raises(monkeypatch): ... def describe_catalog_api_sync(name, client, environment): raise RuntimeError("boom") ... # assert fallback to _has_pyiceberg_catalog_config / load_catalog🤖 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 `@tests/tower/test_tables.py` around lines 174 - 178, Add a test that covers the `describe_catalog` failure path in the Tower table catalog flow. In `describe_catalog_api_sync`, simulate an exception (for example a network/timeout failure) instead of only returning `None` or a `DescribeCatalogResponse`. Verify `_describe_tower_catalog_type` in `src/tower/_storage.py` takes the `except Exception` branch and falls back to the PyIceberg catalog config path by checking `_has_pyiceberg_catalog_config` / `load_catalog` behavior.
🤖 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 `@src/tower/_storage.py`:
- Around line 132-173: The Tower catalog lookup in _describe_tower_catalog_type
can hang indefinitely because describe_catalog_api.sync is called without a
bounded timeout, and failures are not cached so repeated loads keep retrying.
Update _describe_tower_catalog_type to pass a finite timeout through the Tower
client call path (via _env_client or the sync invocation) and make sure both
exception and ErrorModel fallback paths store a negative result in
_catalog_type_cache for the (ctx.tower_url, name, environment) key, so repeated
failures do not reissue the request.
---
Nitpick comments:
In `@tests/tower/test_tables.py`:
- Around line 174-178: Add a test that covers the `describe_catalog` failure
path in the Tower table catalog flow. In `describe_catalog_api_sync`, simulate
an exception (for example a network/timeout failure) instead of only returning
`None` or a `DescribeCatalogResponse`. Verify `_describe_tower_catalog_type` in
`src/tower/_storage.py` takes the `except Exception` branch and falls back to
the PyIceberg catalog config path by checking `_has_pyiceberg_catalog_config` /
`load_catalog` behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7819fb6-37d0-4419-a7fc-20066b3bb5b1
📒 Files selected for processing (6)
.github/workflows/build-binaries.yml.github/workflows/publish-npm.ymlcrates/tower-package/Cargo.tomlsrc/tower/_storage.pysrc/tower/_tables.pytests/tower/test_tables.py
💤 Files with no reviewable changes (1)
- .github/workflows/publish-npm.yml
Comment on lines
+132
to
+173
| def _describe_tower_catalog_type( | ||
| ctx: TowerContext, name: str, environment: str | ||
| ) -> str | None: | ||
| if not (ctx.api_key or ctx.jwt): | ||
| return None | ||
|
|
||
| cache_key = (ctx.tower_url, name, environment) | ||
| if cache_key in _catalog_type_cache: | ||
| return _catalog_type_cache[cache_key] | ||
|
|
||
| try: | ||
| result = describe_catalog_api.sync( | ||
| name=name, | ||
| client=_env_client(ctx), | ||
| environment=environment, | ||
| ) | ||
| except Exception: | ||
| logger.debug( | ||
| "Failed to describe Tower catalog %r in environment %r; " | ||
| "falling back to PyIceberg catalog configuration detection.", | ||
| name, | ||
| environment, | ||
| exc_info=True, | ||
| ) | ||
| return None | ||
|
|
||
| if isinstance(result, DescribeCatalogResponse): | ||
| catalog_type = result.catalog.type_ | ||
| _catalog_type_cache[cache_key] = catalog_type | ||
| return catalog_type | ||
|
|
||
| if isinstance(result, ErrorModel): | ||
| logger.debug( | ||
| "Tower catalog describe for %r in environment %r returned %s; " | ||
| "falling back to PyIceberg catalog configuration detection.", | ||
| name, | ||
| environment, | ||
| _error_text(result), | ||
| ) | ||
|
|
||
| _catalog_type_cache[cache_key] = None | ||
| return None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether _env_client is called elsewhere with an explicit timeout
rg -n -B2 -A2 '_env_client\(' src/tower/_storage.pyRepository: tower/tower-cli
Length of output: 775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect _env_client and nearby callers for timeout handling
sed -n '1,240p' src/tower/_storage.py | cat -n
# Look for any other timeout-related client construction in this module
rg -n 'timeout|AuthenticatedClient|httpx' src/tower/_storage.pyRepository: tower/tower-cli
Length of output: 9457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the client factory used by _storage.py
sed -n '1,220p' src/tower/_client.py | cat -n
# Find any explicit timeout values passed to _env_client anywhere in the repo
rg -n '_env_client\([^)]*timeout=' srcRepository: tower/tower-cli
Length of output: 9538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how AuthenticatedClient handles the timeout argument
rg -n -A6 -B6 'class AuthenticatedClient|def __init__|timeout=' src/tower/tower_api_client
# If the class is defined in a single file, show just that section
fd 'AuthenticatedClient' src/tower/tower_api_client -t fRepository: tower/tower-cli
Length of output: 11121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the _env_client factory and AuthenticatedClient defaults
sed -n '340,430p' src/tower/_client.py | cat -n
sed -n '139,280p' src/tower/tower_api_client/client.py | cat -nRepository: tower/tower-cli
Length of output: 10185
Pass a bounded timeout to Tower catalog description
AuthenticatedClient forwards timeout=None to httpx.Client, so this describe_catalog_api.sync(...) call can hang indefinitely on an unresponsive Tower API. Since the exception path also doesn’t populate _catalog_type_cache, repeated failures will retry on every load. src/tower/_storage.py:143-156
🤖 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 `@src/tower/_storage.py` around lines 132 - 173, The Tower catalog lookup in
_describe_tower_catalog_type can hang indefinitely because
describe_catalog_api.sync is called without a bounded timeout, and failures are
not cached so repeated loads keep retrying. Update _describe_tower_catalog_type
to pass a finite timeout through the Tower client call path (via _env_client or
the sync invocation) and make sure both exception and ErrorModel fallback paths
store a negative result in _catalog_type_cache for the (ctx.tower_url, name,
environment) key, so repeated failures do not reissue the request.
socksy
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Features
Bug Fixes
Chores