Skip to content
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

HL Python SDK: Support storage id #8742

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented Feb 28, 2025

Closes #8740

Verified with existing unit tests and manual testing

@N-o-Z N-o-Z added area/sdk/python exclude-changelog PR description should not be included in next release changelog python-sdk-changelog labels Feb 28, 2025
@N-o-Z N-o-Z requested a review from a team February 28, 2025 19:10
@N-o-Z N-o-Z self-assigned this Feb 28, 2025
Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

14 passed

Copy link

E2E Test Results - Quickstart

11 passed

Copy link
Contributor

@guy-har guy-har left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks!

@@ -684,6 +685,16 @@ def stat(self) -> ObjectInfo:
self._stats = ObjectInfo(**stat.dict())
return self._stats

def storage_id(self) -> str:
"""
Return the Stat object representing this object
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix documentation

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

Looking great, thanks for handling this.

Blocking only since we need to make a decision about GetConfig before merging this one.

@@ -1,5 +1,5 @@
aenum~=3.1.15
lakefs-sdk>=1.47, < 2
lakefs-sdk>=1.50, < 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this version include our latest changes regarding config defaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the config defaults? How is lakeFS python SDK relevant?
As long as we are not changing the response structure this version is enough to support what we need


def __init__(self, client: Optional[Client] = None):
try:
self._conf = client.sdk_client.config_api.get_config()
self._storage_conf = ServerStorageConfiguration(**self._conf.storage_config.dict())
if self._conf.storage_config_list is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's first either merge either GetConfig fix 1 or fix 2 before merging this one, so that all the clients of GetConfig (this one + lakectl + WebUI) will be aligned.

Copy link
Member Author

Choose a reason for hiding this comment

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

How will either of the changes affect the behavior of the underlying code?
In either way to will behave in the same manner it works today

Copy link
Contributor

Choose a reason for hiding this comment

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

Well these should be fine now.

@N-o-Z N-o-Z requested a review from itaigilo March 3, 2025 13:37
Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

LGTM


def __init__(self, client: Optional[Client] = None):
try:
self._conf = client.sdk_client.config_api.get_config()
self._storage_conf = ServerStorageConfiguration(**self._conf.storage_config.dict())
if self._conf.storage_config_list is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Well these should be fine now.

@N-o-Z N-o-Z enabled auto-merge (squash) March 3, 2025 15:44
@N-o-Z N-o-Z merged commit b87f996 into master Mar 3, 2025
42 checks passed
@N-o-Z N-o-Z deleted the task/python-wrapper-storage-config-8740 branch March 3, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sdk/python exclude-changelog PR description should not be included in next release changelog python-sdk-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HL Python SDK: Support storage config by ID
3 participants