GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix ssl deactivation from catalog properties #2985 by MenelikV · Pull Request #3012 · apache/iceberg-python

Conversation

MenelikV commented Feb 9, 2026

Copy link

Copy Markdown

Contributor

Closes #2985

Rationale for this change

Are these changes tested?

Yes

Are there any user-facing changes?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CharlesAttend reacted with thumbs up emoji

All reactions

👍 1 reaction


          fix ssl deactivation from catalog properties apache#2985

9e3efa8

geruh reviewed

Feb 9, 2026

View reviewed changes

geruh left a comment

Copy link

Copy Markdown

Member

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.

The fix looks correct to me, just have a few comments

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/catalog/test_rest.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

tests/catalog/test_rest.py Outdated

		"cabundle": False,
		}
		}
		with pytest.raises(ValidationError) as _:

geruh Feb 9, 2026

Copy link

Copy Markdown

Member

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.

I think we should directly assert that session.verify is False for extra confidence here. Then we could just do something like the following without the callback logic

catalog = RestCatalog("rest", **{
  "uri": TEST_URI,
  "token": TEST_TOKEN,
  "ssl": {"cabundle": False},
})

assert catalog._session.verify is False

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

MenelikV Feb 10, 2026

Copy link

Copy Markdown

Contributor Author

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.

Okay, works for me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

MenelikV added 2 commits

February 10, 2026 09:17


          fix(lint): fix linting issues

ccc9b4a


          feat(test): simpler test without callback

3e84d24

Fokko approved these changes

Feb 15, 2026

View reviewed changes

Fokko left a comment

Copy link

Copy Markdown

Contributor

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.

Thanks @MenelikV for working on this! This looks good to me, I've left a comment on. style issue, but apart from that this PR looks good to me 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

pyiceberg/catalog/rest/__init__.py

Comment on lines +361 to 362

		if (ssl_ca_bundle := ssl_config.get(CA_BUNDLE)) is not None:
		session.verify = ssl_ca_bundle

Fokko Feb 15, 2026

Copy link

Copy Markdown

Contributor

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.

Nit:

Suggested change

	if (ssl_ca_bundle := ssl_config.get(CA_BUNDLE)) is not None:
	session.verify = ssl_ca_bundle
	if ssl_config.get(CA_BUNDLE) is not None:
	session.verify = ssl_config.get[CA_BUNDLE]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Fokko merged commit 4034455 into apache:main

Feb 18, 2026

11 checks passed

Uh oh!

There was an error while loading. Please reload this page.

kevinjqliu pushed a commit that referenced this pull request

Feb 26, 2026


          Fix SSL deactivation using catalog properties (#3012)

8dee48a

<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
Closes #2985 

# Rationale for this change

## Are these changes tested?
Yes

## Are there any user-facing changes?

<!-- In the case of user-facing changes, please add the changelog label.
-->

Reviewers

geruh

geruh left review comments

Fokko

Fokko approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Could not disable ssl verification.

Uh oh!

There was an error while loading. Please reload this page.

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.