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

Add support for Object Storage Gen 2 #503

Merged
merged 8 commits into from
Feb 5, 2025

Conversation

ezilber-akamai
Copy link
Contributor

@ezilber-akamai ezilber-akamai commented Jan 29, 2025

📝 Description

Added support for OBJ Gen 2. Also fixed issue with cluster_id and region ambiguity.

✔️ How to Test

Unit Tests

make test-unit

Integration Tests

make test-int TEST_SUITE=object_storage

@ezilber-akamai ezilber-akamai requested a review from a team as a code owner January 29, 2025 18:40
@ezilber-akamai ezilber-akamai requested review from jriddle-linode and zliang-akamai and removed request for a team January 29, 2025 18:40
@lgarber-akamai lgarber-akamai self-requested a review February 4, 2025 18:12
Comment on lines 331 to 332
s3_endpoint: str = None,
endpoint_type: ObjectStorageEndpointType = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these two be given Optional[...] types to be consistent with their default values?



@dataclass
class ObjectStorageEndpoint(JSONObject):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment on lines 90 to 93
if json is not None:
cluster_or_region = json.get("region") or json.get("cluster")
if parent_id is None and cluster_or_region:
parent_id = cluster_or_region
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: This snippet can be simplified a bit 🙂

Suggested change
if json is not None:
cluster_or_region = json.get("region") or json.get("cluster")
if parent_id is None and cluster_or_region:
parent_id = cluster_or_region
if json is not None:
parent_id = parent_id or json.get("region") or json.get("cluster")

Copy link
Member

Choose a reason for hiding this comment

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

Nice simplification!

Copy link
Contributor

@lgarber-akamai lgarber-akamai left a comment

Choose a reason for hiding this comment

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

All tests are passing on my end and the implementation looks great other than a small style suggestion. Great work!

Copy link
Member

@zliang-akamai zliang-akamai left a comment

Choose a reason for hiding this comment

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

LGTM!

@ezilber-akamai ezilber-akamai merged commit b23ac9e into linode:dev Feb 5, 2025
12 checks passed
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