-
Notifications
You must be signed in to change notification settings - Fork 127
update storage extension to v2.0.0 #1561
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
base: main
Are you sure you want to change the base?
update storage extension to v2.0.0 #1561
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1561 +/- ##
==========================================
- Coverage 92.29% 92.27% -0.02%
==========================================
Files 55 55
Lines 8395 8519 +124
Branches 971 984 +13
==========================================
+ Hits 7748 7861 +113
- Misses 458 468 +10
- Partials 189 190 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gadomski
left a comment
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 have unnecessary new extensions here ... both schemes and refs can just be classes, not extensions themselves?
| class StorageSchemeType(StringEnum): | ||
| AWS_S3 = "aws-s3" | ||
| CUSTOM_S3 = "custom-s3" | ||
| AZURE = "ms-azure" |
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.
Can you also add GCS = "gcp-gs"
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.
generally, having parity to the v1 Enum would be great
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 didn't see best practice GCP storage defined here, so did not add as a possible enum here, but as written schemes can be set to any string. I think opening a PR in the extension w/ a suggested schema to use for GCP storage would make sense before implemented here.
1b0a9ab to
a0267df
Compare
Description:
Updates storage extension to v2.0.0. Due to the major changes in v2, I have not attempted to make migrations from v1 to v2
StorageScheme platform property allows templated uri's, so arbitrary additional properties are allowed to be set on StorageScheme objects
PR Checklist:
pre-commit run --all-files)pytest)