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

Move storage blobs to pipeline architecture #843

Merged
merged 26 commits into from
Jun 23, 2022

Conversation

bmc-msft
Copy link
Contributor

No description provided.

Brian Caswell added 21 commits June 21, 2022 12:27
Changes beyond the into_future pattern updates:
1. This updates ContentDisposition, ContentEncoding, ContentLanguage, and ContentType to follow other header types.
2. This adds a `BlobClient.get_content` helper that iterates over `BlobClient.get().stream()` and writes the data into a single Vec, returning the Vec.  This was an extremely common pattern in the examples and something akin to it is available in most of the other SDKs as well.

TODO:
* move container operations to the into_future model
@bmc-msft
Copy link
Contributor Author

Note, this does not move get_blob, as that implements paging by shifting the Range for the blob. The current Pageable support only supports Continuation as a String, while this needs the current offset.

@bmc-msft bmc-msft requested a review from rylev June 22, 2022 21:04
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Reviewed together with @yoshuawuyts 🧡

There are some small things that we think we should change, but feel free to do those in a follow up PR if you want.

Comment on lines +11 to +16
use azure_core::{
auth::TokenCredential,
error::{Error, ErrorKind, ResultExt},
headers::*,
ClientOptions, Context, HttpClient, Pipeline, Request, Response,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we don't seem to have a consistent way of structuring imports. I'd like to figure out a consistent way of structuring these things and stick to that across all of the SDKs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, preferably using imports_granularity once it stabilizes.

sdk/storage_blobs/examples/container_00.rs Outdated Show resolved Hide resolved
sdk/storage_blobs/examples/list_blobs_00.rs Outdated Show resolved Hide resolved
sdk/storage_blobs/src/blob/operations/put_block.rs Outdated Show resolved Hide resolved
sdk/storage_blobs/src/blob/operations/put_block_blob.rs Outdated Show resolved Hide resolved
sdk/storage_blobs/src/clients/blob_service_client.rs Outdated Show resolved Hide resolved
let storage_account_client =
StorageAccountClient::new_access_key(http_client, &account, &account_key);

let blob_service_client = storage_account_client.as_blob_service_client();
Copy link
Contributor

Choose a reason for hiding this comment

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

In Cosmos, we would use the naming convention storage_account_client.blob_client(). This was discussed in a previous PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do this change as a follow-up PR, as I'd like feedback on it uniquely.

@bmc-msft bmc-msft merged commit 8586a66 into Azure:main Jun 23, 2022
@bmc-msft bmc-msft deleted the move-storage-blobs-to-pipeline branch June 23, 2022 14:18
@cataggar cataggar mentioned this pull request Jun 27, 2022
9 tasks
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.

2 participants