Cache folder configuration for Azure and AWS Image Caches (Lombiq Technologies: OCORE-136) #353

Piedone · 2024-01-17T00:15:40Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

For multi-tenancy scenarios and for feature parity with PhysicalFileSystemCache, this PR adds a CacheFolder configuration for the Azure and AWS Image Caches

Please see #351 for more details, especially the rationale.

There are two things I'm unsure about, please advise:

Tests: My usual approach in such cases would be to turn the test into a Theory with input parameters, and run the test once without setting CacheFolder, and then once with it. In this test suite, I don't really see how to approach this; I could add another class like AzureBlobStorageCacheTestServerFixture where the new property is set, but that seems like the wrong approach. Same for AWSS3StorageCacheTestServerFixture. What should I do? I've run the tests with the CacheFolders set and they still pass BTW.
For AWS, I added the new property only to AWSS3StorageCacheOptions instead of also to IAWSS3BucketClientOptions, since the scope of Ability to configure a folder (sub-path) for Azure Blob Storage and AWS S3 caches #351 is about caching only. Should I do anything differently?

See the related docs PR: SixLabors/docs#65.

Thank you!

CLAassistant · 2024-01-17T00:15:46Z

All committers have signed the CLA.

Piedone · 2024-01-17T00:18:49Z

src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCache.cs

+        this.cacheFolder = string.IsNullOrEmpty(options.CacheFolder)
+            ? string.Empty
+            : options.CacheFolder.Trim().Trim('/') + '/';


Should I DRY this into a helper class, not to copy it between the two cache implementations?

I would leave as is and keep them separate. They're two separate assemblies.

Piedone · 2024-01-17T00:19:20Z

src/ImageSharp.Web.Providers.Azure/Caching/AzureBlobStorageCache.cs

    }

    /// <inheritdoc/>
    public async Task<IImageCacheResolver?> GetAsync(string key)
    {
-        BlobClient blob = this.container.GetBlobClient(key);
+        BlobClient blob = this.container.GetBlobClient(this.GetBlobName(key));


Should instead perhaps the whole right side be a method, like GetBlob()?

I refactored this.

codecov · 2024-01-24T09:01:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6096704) 85% compared to head (ff7fdc4) 85%.

Additional details and impacted files

@@         Coverage Diff         @@
##           main   #353   +/-   ##
===================================
  Coverage    85%    85%           
===================================
  Files        82     82           
  Lines      2332   2343   +11     
  Branches    348    350    +2     
===================================
+ Hits       1999   2012   +13     
+ Misses      231    230    -1     
+ Partials    102    101    -1

Flag	Coverage Δ
unittests	`85% <100%> (+<1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JimBobSquarePants

Thanks for this. Just some minor issues here.

Regarding tests, it's not easy to make the configuration optional. For now, spin up a new fixture.

JimBobSquarePants · 2024-01-24T08:57:22Z

src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCache.cs

+        this.cacheFolder = string.IsNullOrEmpty(options.CacheFolder)
+            ? string.Empty
+            : options.CacheFolder.Trim().Trim('/') + '/';


I would leave as is and keep them separate. They're two separate assemblies.

JimBobSquarePants · 2024-01-24T08:58:44Z

src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCacheOptions.cs

+    /// <summary>
+    /// Gets or sets the cache folder's name that'll store cache files under the configured bucket.
+    /// </summary>
+    public string CacheFolder { get; set; } = null!;


Don't we want this to be nullable?

We actually do. I changed this for the Azure class too.

JimBobSquarePants · 2024-01-24T08:59:11Z

src/ImageSharp.Web.Providers.Azure/Caching/AzureBlobStorageCacheOptions.cs

+    /// Must conform to Azure Blob Storage directory naming guidelines.
+    /// <see href="https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#directory-names"/>
+    /// </summary>
+    public string CacheFolder { get; set; } = null!;


Don't we want this to be nullable?

Piedone · 2024-02-21T22:46:13Z

Could you check this out @JimBobSquarePants, please?

JimBobSquarePants · 2024-02-22T04:51:07Z

Sorry, yep will do!

Piedone · 2024-02-22T13:50:51Z

Thank you!

JimBobSquarePants

This looks great. Thanks!

Piedone · 2024-03-06T17:50:54Z

Awesome! And thanks for the quick release!

Piedone added 4 commits January 17, 2024 00:48

Adding CacheFolder config to AzureBlobStorageCache

98a43eb

Grammar

5ff144c

Adding CacheFolder config to AWSS3StorageCache

40b5b8c

Code styling

9db0e6b

Piedone mentioned this pull request Jan 17, 2024

Adding docs about the new CacheFolder config for Azure and AWS Image Caches SixLabors/docs#65

Merged

Piedone commented Jan 17, 2024

View reviewed changes

Piedone mentioned this pull request Jan 17, 2024

Feature to use the Azure or AWS-specific Image Caches of ImageSharp OrchardCMS/OrchardCore#15016

Closed

JimBobSquarePants reviewed Jan 24, 2024

View reviewed changes

Piedone added 4 commits January 25, 2024 00:15

DRY

408610b

Adding Azure CacheFolder tests

dbcae3a

Adding AWS tests

e41bc2a

The CacheFolder properties should actually be nullabe

ff7fdc4

Piedone requested a review from JimBobSquarePants January 24, 2024 23:32

Piedone changed the title ~~Cache folder configuration for Azure and AWS Image Caches~~ Cache folder configuration for Azure and AWS Image Caches (Lombiq Technologies: OCORE-136) Mar 1, 2024

JimBobSquarePants approved these changes Mar 6, 2024

View reviewed changes

JimBobSquarePants merged commit a3f306c into SixLabors:main Mar 6, 2024
9 checks passed

Piedone mentioned this pull request Mar 8, 2024

Adding Azure and AWS-specific Image Caches of ImageSharp (Lombiq Technologies: OCORE-136) OrchardCMS/OrchardCore#15028

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache folder configuration for Azure and AWS Image Caches (Lombiq Technologies: OCORE-136) #353

Cache folder configuration for Azure and AWS Image Caches (Lombiq Technologies: OCORE-136) #353

Piedone commented Jan 17, 2024 •

edited

Loading

CLAassistant commented Jan 17, 2024 •

edited

Loading

Piedone Jan 17, 2024

JimBobSquarePants Jan 24, 2024

Piedone Jan 24, 2024

Piedone Jan 17, 2024

Piedone Jan 24, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

JimBobSquarePants left a comment

JimBobSquarePants Jan 24, 2024

JimBobSquarePants Jan 24, 2024

Piedone Jan 24, 2024

JimBobSquarePants Jan 24, 2024

Piedone commented Feb 21, 2024

JimBobSquarePants commented Feb 22, 2024

Piedone commented Feb 22, 2024

JimBobSquarePants left a comment

Piedone commented Mar 6, 2024

Cache folder configuration for Azure and AWS Image Caches (Lombiq Technologies: OCORE-136) #353

Cache folder configuration for Azure and AWS Image Caches (Lombiq Technologies: OCORE-136) #353

Conversation

Piedone commented Jan 17, 2024 • edited Loading

Prerequisites

Description

CLAassistant commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piedone commented Feb 21, 2024

JimBobSquarePants commented Feb 22, 2024

Piedone commented Feb 22, 2024

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Piedone commented Mar 6, 2024

Piedone commented Jan 17, 2024 •

edited

Loading

CLAassistant commented Jan 17, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading