Skip to content

Conversation

@bdilday
Copy link
Contributor

@bdilday bdilday commented Oct 6, 2023

I found that I was unable to change a catalog URI after it had been configured by an environment variable. Example,

import os
os.environ["PYICEBERG_CATALOG__SOMEDB__URI"] = "https://service-one.io"
from pyiceberg.catalog import load_catalog
catalog = load_catalog("somedb", uri="https://service-two.io")

[tries to connect to https://service-one.io]

I think the more intuitive behavior is that the URI would be initialized based on the environment variable (if present) but that the argument passed to load_catalog would be able to override the value. This PR updates the behavior so that the passed in argument takes precedence, by swapping the lhs and rhs preference in the merge_config method.

@rdblue
Copy link
Contributor

rdblue commented Oct 8, 2023

The proposed behavior sounds reasonable to me.

@Fokko
Copy link
Contributor

Fokko commented Oct 8, 2023

Thanks @bdilday for raising this, and I agree that the explicitly passed-in configuration should take preference. Can you rebase against the latest main branch? The CI should turn green 👍

@bdilday bdilday force-pushed the bf-config-rhs-override branch from ae0f845 to e30c690 Compare October 9, 2023 18:02
@Fokko Fokko added this to the PyIceberg 0.6.0 release milestone Oct 10, 2023
@bdilday bdilday force-pushed the bf-config-rhs-override branch 3 times, most recently from 0e57e95 to ba1a6f4 Compare October 11, 2023 14:29
@bdilday bdilday force-pushed the bf-config-rhs-override branch 2 times, most recently from 9a7fb83 to 829705f Compare October 12, 2023 14:51
@bdilday bdilday force-pushed the bf-config-rhs-override branch from 829705f to 2fc335a Compare October 12, 2023 14:52
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @bdilday

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