Skip to content
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

Set the CacheControl property on stored blobs #6297

Merged
merged 4 commits into from
Aug 17, 2018
Merged

Conversation

loic-sharma
Copy link
Contributor

@loic-sharma loic-sharma commented Aug 10, 2018

This sets the CacheControl property on all packages blobs to 2 minutes.

Fixes #6285

@@ -318,6 +318,7 @@ public async Task SaveFileAsync(string folderName, string fileName, Stream file,
}

blob.Properties.ContentType = GetContentType(folderName);
blob.Properties.CacheControl = CoreConstants.DefaultCacheControl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this adding cache control to all files saved to blob storage in the gallery? Or just packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This affects all uploaded blobs for all blob containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to only affect the packages container.

switch (folderName)
{
case CoreConstants.PackagesFolderName:
return CoreConstants.DefaultCacheControl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: PackageCacheControl instead of Default

@@ -549,6 +550,7 @@ private static string GetContentType(string folderName)
case CoreConstants.PackageReadMesFolderName:
return CoreConstants.TextContentType;

case CoreConstants.ContentFolderName:
case CoreConstants.RevalidationFolderName:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this addition? A test should cover it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The the content type tests skipped ContentFolderName, which caused it to fail my new test. I'll fix the content type test.

@loic-sharma loic-sharma merged commit f2436a7 into dev Aug 17, 2018
@loic-sharma loic-sharma deleted the loshar-cache-control branch September 11, 2018 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants