Feature/app center distribute chunked symbol files #10539
Conversation
|
What is the reason that we want to update all the versions? Can we live with asking customers to update the task version if they need to upload large files? I'm assuming most of the work is copy paste. But this means we need to release three times instead of once. Any future changes will have to be done on all the versions too. |
| "x-ms-blob-type": "BlockBlob", | ||
| "Content-Length": stat.size, | ||
| "User-Agent": userAgent, | ||
| "internal-request-source": "VSTS" |
There was a problem hiding this comment.
Looks like we will lose some of the headers like User-Agent and internal-request-source. But if I'm understanding correctly, this has always been the blob url, so these haven't been used before
There was a problem hiding this comment.
@dennispan Yeah. Given that we're uploading directly to Azure before they probably weren't used. Is there anything special about these headers that we should know about?
There was a problem hiding this comment.
They were used for telemetry purpose. But looks like this particular call wasn't needed given it doesn't go through App Center
|
|
||
| import { inspect } from "util"; | ||
|
|
||
| export class AzureBlobUploadHelper { |
There was a problem hiding this comment.
Do we have tests for this?
There was a problem hiding this comment.
We don't at the moment. This code was borrowed from the App Center CLI and we didn't find any matching tests. We mostly weren't sure how they would fit into the rest of the test suite. But we can try to retrofit if it's important.
There was a problem hiding this comment.
Ideally we should have tests for these? If it's significant amount of effort, we probably have to skip. I would suggest investigate what it takes though.
There was a problem hiding this comment.
Given that it's basically some glue code wrapping the Azure Storage SDK, we're going to skip adding tests for now as we don't think the ROI is worth the effort.
|
@dennispan Given that this affects existing customers, we wanted to add the fix to all of the existing versions to unblock them. Are we missing anything? |
|
@pfleidi I understand where you are coming from. I'm also fine with the approach. Just want to point out that we can also choose to update only the latest version and ask customers to upgrade. This is likely to the best interest of the customers, because they have a better chance of using something that has better support (not on symbol upload, but on what distribution api we use, etc.). If we see many customers running into failures in upload large symbols, then it makes sense to update all versions. If the number is low, then asking customers to upgrade is something worth considering (again, not trying to push us to go with this; just an option to call out). |
|
@dennispan I'm fine with it either way. We could revert the changes in v1 and v2 and only fix v3. Most of the code was copy/pasted anyways. @cainejette: What do you think? |
…nto feature/app-center-distribute-chunked-symbol-files
|
@dennispan @pfleidi I think we should update all versions. Yes, it is a copy paste into each version. This is what a customer obsessed engineer would do rather than force customers to upgrade their version. |
cainejette
force-pushed
the
feature/app-center-distribute-chunked-symbol-files
branch
from
28f065e to
dacdf85
Compare
In this PR, we are updating all versions of the App Center distribute tasks.
Earlier versions of these tasks have an issue with uploading larger (> 256mb) symbol files. Because we are uploading files directly to Azure Blob Storage, symbol files larger than 256mb will be rejected by Azure.
The changes in this PR leverage the Azure storage SDK to automatically chunk file uploads if the file are too large. We borrowed a helper class from the App Center CLI which takes care of uploading and updated the tests accordingly.