Added immutableCacheControl and mutableCacheControl configuration#232
Added immutableCacheControl and mutableCacheControl configuration#232hajekj wants to merge 13 commits intoemgarten:mainfrom
immutableCacheControl and mutableCacheControl configuration#232Conversation
… options for Azure Storage and Amazon S3 feeds
There was a problem hiding this comment.
Pull request overview
Adds configurable Cache-Control behavior for immutable vs. mutable feed artifacts to better support CDN proxy scenarios for Azure Blob Storage and Amazon S3 backends.
Changes:
- Introduces
immutableCacheControlandmutableCacheControlsource settings insleet.jsonand wires them through factory creation. - Applies the configured cache-control values during Azure blob uploads and S3 object uploads based on file type.
- Documents the new settings and adds release notes for the new version entry.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SleetLib/FileSystem/FileSystemFactory.cs | Reads new cache-control settings from config and passes them into Azure/S3 filesystem constructors. |
| src/SleetLib/FileSystem/AzureFileSystem.cs | Stores cache-control settings and passes them to AzureFile instances. |
| src/SleetLib/FileSystem/AzureFile.cs | Sets BlobHttpHeaders.CacheControl per file type (immutable vs mutable) during upload. |
| src/SleetLib/FileSystem/AmazonS3FileSystemAbstraction.cs | Extends upload API to accept a cache-control value for S3 uploads. |
| src/SleetLib/FileSystem/AmazonS3FileSystem.cs | Stores cache-control settings and passes them into AmazonS3File. |
| src/SleetLib/FileSystem/AmazonS3File.cs | Selects cache-control per file type and forwards it to the S3 upload abstraction. |
| doc/client-settings.md | Documents the new caching configuration options and guidance. |
| ReleaseNotes.md | Adds a 7.1.0 entry describing the new cache-control configuration options. |
Comments suppressed due to low confidence (1)
src/SleetLib/FileSystem/AmazonS3File.cs:158
- In the unknown file type branch, the upload still proceeds with whatever
cacheControlwas initialized to (currentlyno-cache). Since this is explicitly an unknown classification, it would be safer/more consistent to fall back to the conservative setting (e.g.,no-store) rather than enabling cacheability for unclassified content.
else
{
log.LogWarning($"Unknown file type: {absoluteUri}");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I mistakenly used to |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Introduce CacheControlTests for Amazon S3 and Azure backends to verify correct Cache-Control headers on uploaded blobs/files. Tests cover default ("no-store") and custom cache control values for both immutable and mutable files, using both direct file system construction and settings-based configuration. Ensures Sleet sets headers as expected for both storage providers.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : this(cache, root, root, blobServiceClient, container) | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
The public AzureFileSystem constructor signature was changed by adding new parameters. Even though they’re optional, this is a binary-breaking change for any existing compiled consumers calling the previous ctor overload. Consider restoring the old ctor signature and adding a new overload that accepts the cache-control parameters (and delegates to a shared private ctor) to preserve binary compatibility.
| // Backwards-compatible constructor matching the original public signature | |
| public AzureFileSystem(LocalCache cache, Uri root, Uri baseUri, BlobServiceClient blobServiceClient, string container, string? feedSubPath = null) | |
| : this(cache, root, baseUri, blobServiceClient, container, feedSubPath, null, null) | |
| { | |
| } |
| @@ -34,14 +36,18 @@ public AmazonS3FileSystem(LocalCache cache, | |||
| string? feedSubPath = null, | |||
| bool compress = true, | |||
| S3CannedACL? acl = null, | |||
There was a problem hiding this comment.
The public AmazonS3FileSystem ctor signature was modified to include cache-control parameters. Adding parameters (even optional ones) is a binary-breaking change for consumers compiled against the previous signature. To avoid an unintended breaking change in a minor release, consider keeping the existing ctor and adding a new overload (or a separate options type) that includes the new cache-control settings.
| S3CannedACL? acl = null, | |
| S3CannedACL? acl = null, | |
| bool disablePayloadSigning = false) | |
| : this(cache, root, baseUri, client, bucketName, serverSideEncryptionMethod, feedSubPath, compress, acl, disablePayloadSigning, immutableCacheControl: null, mutableCacheControl: null) | |
| { | |
| } | |
| public AmazonS3FileSystem(LocalCache cache, | |
| Uri root, | |
| Uri baseUri, | |
| IAmazonS3 client, | |
| string bucketName, | |
| ServerSideEncryptionMethod serverSideEncryptionMethod, | |
| string? feedSubPath = null, | |
| bool compress = true, | |
| S3CannedACL? acl = null, |
| public static async Task UploadFileAsync( | ||
| IAmazonS3 client, | ||
| string bucketName, | ||
| string key, | ||
| string? contentType, | ||
| string? contentEncoding, | ||
| Stream reader, | ||
| ServerSideEncryptionMethod serverSideEncryptionMethod, | ||
| S3CannedACL? acl, | ||
| bool disablePayloadSigning, | ||
| string? cacheControl, | ||
| CancellationToken token) |
There was a problem hiding this comment.
AmazonS3FileSystemAbstraction.UploadFileAsync is a public API and its signature was changed by adding a new parameter. This is binary-breaking for existing compiled callers. Consider adding an overload with the new cacheControl parameter while keeping the old signature (defaulting to no-store) for backward compatibility.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@emgarten - added tests. Regarding the signature changes, do you want me to address that too? I was able to run the Azure tests only, as I don't have any S3 account at my disposal. |
I opted in to implement cache configuration for files which are considered immutable - they are tied to a specific version (in their versioned folder) or a hash (pdbs) and files which are mutable (json and badges) so that we can have a little more control over how this is handled when the feed is proxied through a CDN to improve performance and optimize transfer costs.
Implemented it for both Azure and S3. The defaults are left as is -
no-store.Resolves #231