[storage] Add azure_storage_sas crate for User Delegation SAS generation#4462
[storage] Add azure_storage_sas crate for User Delegation SAS generation#4462eureka-cpu wants to merge 1 commit into
azure_storage_sas crate for User Delegation SAS generation#4462Conversation
eureka-cpu
requested review from
LarryOsterman,
RickWinter,
heaths,
jalauzon-msft,
ronniegeraghty,
vincenttran-msft and
xirzec
as code owners
github-actions
Bot
added
Community Contribution
|
Thank you for your contribution @eureka-cpu! We will review the pull request and get back to you soon. |
eureka-cpu
force-pushed
the
eureka-cpu/4444
branch
from
be8f9b9 to
0e6014a
Compare
heaths
left a comment
heaths
left a comment
There was a problem hiding this comment.
Thanks for your contribution; however, there are a number of guideline violations in the public API. I'll let @vincenttran-msft and @jalauzon-msft review the Storage-specific functionality, but I don't think you need or should have your own errors. We have strict guidelines on those, and typically only add more specific errors if 1) they can yield more actionable information (we don't define error kinds just because it's useful; it's not helpful if you can't act on it when a message is sufficient), and 2) errors need to provide more structured information than the general azure_core::Error does. Storage already has its own error, for example, that - if necessary - could be put in azure_storage_common and that published going forward if they want to share it across crates.
| "sdk/canary/azure_canary", | ||
| "sdk/storage/azure_storage_blob", | ||
| "sdk/storage/azure_storage_queue", | ||
| "sdk/storage/azure_storage_sas" |
| @@ -0,0 +1,31 @@ | |||
| #[derive(Debug, thiserror::Error)] | |||
| pub enum SasError { | |||
There was a problem hiding this comment.
We don't use thiserror. You should return azure_core::Error in most cases. If a subordinate crate has good reason to define it's own, it must still follow the guidelines for errors - no enums (they are problematic) - and be convertible to/from azure_core::Error.
| reqwest = { workspace = true, default-features = false, features = ["json", "rustls"] } | ||
| quick-xml.workspace = true | ||
| serde.workspace = true | ||
| thiserror.workspace = true |
There was a problem hiding this comment.
Don't use thiserror. Didn't even realize we had this. Will remove it.
Also, how are all these dependencies necessary? I don't see how serde and quick-xml are necessary. Even reqwest - which we cannot depend on unconditionally anyway - shouldn't be necessary to create a SAS URI.
| /// Re-export of the [`time`] crate. | ||
| pub use time; | ||
| /// Re-export of the [`url`] crate. | ||
| pub use url; |
There was a problem hiding this comment.
Both are already re-exported from azure_core and should not be here as well. Use them from azure_core, in fact, not from their original crates in case we ever re-implement them to add needed functionality we can't upstream.
| /// } | ||
| /// ``` | ||
| /// | ||
| /// <https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas> |
There was a problem hiding this comment.
URLs should never have a locale in them. Covered in general guidelines.
| /// <https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas> | |
| /// <https://learn.microsoft.com/rest/api/storageservices/create-user-delegation-sas> |
|
Much appreciated on the feedback @heaths! Thank you, I will fix these up shortly. |
Closes #3330
Closes #4444
Adds a new crate which adds type safety to user delegation SAS URL generation for blob storage, files, queues and tables.