Specify Content-Type for BlockBlob upload
#3119
Conversation
| @@ -234,6 +234,7 @@ private async Task UploadBlockFileAsync(string url, string blobStorageType, File | |||
| if (blobStorageType == BlobStorageTypes.AzureBlobStorage) | |||
| { | |||
| request.Content.Headers.Add(Constants.AzureBlobTypeHeader, Constants.AzureBlockBlob); | |||
| request.Content.Headers.Add("Content-Type", "text/plain"); | |||
There was a problem hiding this comment.
Do we need to update the SDK path? Or that's handled automatically for us? In that case, once we rollout the SDK change, does this become a none issue?
There was a problem hiding this comment.
Added to the sdk case, thank you!
| try | ||
| { | ||
| await blobClient.UploadAsync(file, cancellationToken); | ||
| await blobClient.UploadAsync(file, uploadOptions, cancellationToken); |
There was a problem hiding this comment.
do you need to check what is the default option is and merge with it instead of just overwrite it?
There was a problem hiding this comment.
Great call-out - this was overwriting the headers.
There was a problem hiding this comment.
not quite sure i follow your new change.
my initial comment is basically try to make sure the SDK don't have any special initialized BlobUploadOptions.
Based on the source code, maybe this is what you want?
Not sure... we can chat more...
await blobClient.UploadAsync(file, new BlobUploadOptions()
{
HttpHeaders = new BlobHttpHeaders
{
ContentType = "text/plain"
},
Conditions = new BlobRequestConditions
{
IfNoneMatch = new ETag("*")
}
}, cancellationToken);
There was a problem hiding this comment.
Gotcha! That makes sense to me. I was thinking specifically around the HTTP headers but I do recall seeing that in the source code of the SDK when I looked.
There was a problem hiding this comment.
Found this issue which was helpful for wrapping my head around the options: Azure/azure-sdk-for-net#9823. Thanks for calling it out!
Content-Type to BlockBlob uploadContent-Type for BlockBlob upload
|
@bethanyj28 How are we handling blob downloads that actually should have a content type of |
| if (extension == ".txt") | ||
| { | ||
| uploadOptions.HttpHeaders.ContentType = "text/plain"; | ||
| } |
There was a problem hiding this comment.
@bethanyj28 How are we handling blob downloads that actually should have a content type of
application/octet-stream? For example, we want this for downloading raw logs.
@jtamsut this is the way we chatted about earlier, let me know if you have something else in mind.
There was a problem hiding this comment.
Ah I missed this. Okay this looks good to me!
| IfMatch = new ETag("*") | ||
| }; | ||
| string extension = System.IO.Path.GetExtension(file.Name); | ||
| if (extension == ".txt") |
There was a problem hiding this comment.
is this check required, since we don't have this in the non-SDK version?
There was a problem hiding this comment.
This is needed to support #3114 since those should be of type application/octet-stream. I didn't add it to the non-sdk version since that should be rolled out by the time the diagnostic change is out, but happy to add to both if I misunderstood! CC @yacaovsnc
There was a problem hiding this comment.
I am not quite sure whether we use .txt in the runner, ex: are those log file ends up with .txt?
There was a problem hiding this comment.
Yep! Our log files our .txt (that's the job-logs.txt). The diagnostic logs are .zip.
Also have validated the change that it does set the content-type correctly.
There was a problem hiding this comment.
Chatted offline and updated to not use the runner file extension, but the blob url instead!
| { | ||
| IfNoneMatch = new ETag("*") | ||
| }; | ||
| if (url.Contains(".txt")) // check if blob path is .txt file |
There was a problem hiding this comment.
is it possible to let the caller to decide what content type to use?
Ex:
private async Task UploadBlockFileAsync(string url, string blobStorageType, FileStream file, bool plainTextContent, CancellationToken cancellationToken)
The default content type for BlockBlob uploads is
application/octet-stream, which signals to many browsers that they should download the content. This adds the content type oftext/plainto hint that the browser should open the content in a new tab.Closes https://github.com/github/actions-results-team/issues/2137