-
Notifications
You must be signed in to change notification settings - Fork 921
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great call-out - this was overwriting the headers.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this check required, since we don't have this in the non-SDK version?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/plain
to hint that the browser should open the content in a new tab.Closes https://github.com/github/actions-results-team/issues/2137