Remove WithCPK and thread options down the hierarchy#8277
Conversation
tg-msft
requested review from
JoshLove-msft,
amnguye,
pakrym and
seanmcc-msft
| using Azure.Storage.Blobs.Models; | ||
| using Azure.Storage.Blobs.Specialized; | ||
|
|
||
| namespace Azure.Storage |
There was a problem hiding this comment.
Shouldn't this be in Test namespace?
There was a problem hiding this comment.
It doesn't matter much and it's an existing partial class. I'm trying to touch as few files as possible with all the big changes in flight.
| /// </param> | ||
| internal BlobServiceClient(Uri serviceUri, HttpPipelinePolicy authentication, BlobClientOptions options) | ||
| : this(serviceUri, authentication, options.Build(authentication), new ClientDiagnostics(options)) | ||
| : this(serviceUri, authentication, options.Build(authentication), new ClientDiagnostics(options), options?.CustomerProvidedKey) |
There was a problem hiding this comment.
Can options be null here? If so, there would already be a bug in the options.Build call.
There was a problem hiding this comment.
No - everyone who calls this does options ?? new BlobClientOptions().
There was a problem hiding this comment.
So can you update to be options.CustomerProvidedKey?
There was a problem hiding this comment.
I could, but this is still safer so I'd prefer to leave it.
|
Do we have coverage for all the ctors? Especially around options being nullable? I've broken it once. Otherwise change LGTM |
|
/azp run net - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Yes, we've got .ctor coverage here. It might not be completely exhaustive, but it's pretty decent because we flow the client hierarchy a lot and call these particular overloads. |
4 participants
Fixes #7830