Skip to content

fix: correctly detect which catalogs we should vend credentials for#313

Merged
konstantinoscs merged 1 commit into
developfrom
fix/tow-2325-regression-in-0368-causes-s3tables-catalogs-to-incorrectly
Jul 3, 2026
Merged

fix: correctly detect which catalogs we should vend credentials for#313
konstantinoscs merged 1 commit into
developfrom
fix/tow-2325-regression-in-0368-causes-s3tables-catalogs-to-incorrectly

Conversation

@konstantinoscs

Copy link
Copy Markdown
Contributor

The problem was that we try to vend credentials for all tables in the tables() call so it messed up s3 tables. This fixes that

@konstantinoscs konstantinoscs requested a review from bradhe July 1, 2026 22:10
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5b8cf5a0-ff53-4702-a548-5c0d9feedbab

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/tow-2325-regression-in-0368-causes-s3tables-catalogs-to-incorrectly

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

Comment thread src/tower/_storage.py
Comment on lines +146 to +147
except Exception:
return None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to get this out for diagnostic purposes somehow? I'm not really sure how this call could fail.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean? If we want to report it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

like if you hit this exception let's log something or die louder rather than swallow it and pretend nothing went wrong, since it shouldn't break in usual usage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a log here. But we should not die loudly:

We use this function to answer: “Is this string catalog a Tower-managed tower-catalog, so we should vend Tower credentials, or is it BYO/S3 Tables, so we should use PyIceberg config?”
If the probe fails, we still have a valid fallback path: check for PyIceberg config and then call load_catalog(). That’s especially important for BYO/S3 Tables customers, because their catalog may work perfectly through runner-injected PYICEBERG_CATALOG__... config even if Tower’s describe API is unavailable, unauthorized, stale, or temporarily failing.

Comment thread src/tower/_tables.py
@konstantinoscs konstantinoscs force-pushed the fix/tow-2325-regression-in-0368-causes-s3tables-catalogs-to-incorrectly branch from 8066fe8 to dd39347 Compare July 2, 2026 11:34
@konstantinoscs konstantinoscs merged commit d830fab into develop Jul 3, 2026
28 checks passed
@konstantinoscs konstantinoscs deleted the fix/tow-2325-regression-in-0368-causes-s3tables-catalogs-to-incorrectly branch July 3, 2026 09:10
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.

3 participants