Adding Azure and AWS-specific Image Caches of ImageSharp (Lombiq Technologies: OCORE-136) #15028
Conversation
|
Could you please review this first draft @MikeAlhayek while I'm waiting for the ImageSharp PR to be approved? |
src/OrchardCore.Modules/OrchardCore.Media.Azure/Helpers/FluidParserHelper.cs
Outdated
Show resolved
Hide resolved
...rdCore.Modules/OrchardCore.Media.Azure/Services/AzureBlobStorageCacheOptionsConfiguration.cs
Outdated
Show resolved
Hide resolved
...ardCore.Modules/OrchardCore.Media.Azure/Services/ImageSharpImageCacheOptionsConfiguration.cs
Outdated
Show resolved
Hide resolved
...ardCore.Modules/OrchardCore.Media.Azure/Services/ImageSharpImageCacheOptionsConfiguration.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Media.Azure/Services/ImageSharpImageCacheTenantEvents.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Media.Azure/Services/MediaBlobContainerTenantEvents.cs
Outdated
Show resolved
Hide resolved
...OrchardCore.Modules/OrchardCore.Media.Azure/Services/MediaBlobStorageOptionsConfiguration.cs
Outdated
Show resolved
Hide resolved
...OrchardCore.Modules/OrchardCore.Media.Azure/Services/MediaBlobStorageOptionsConfiguration.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # src/OrchardCore.Build/Dependencies.props # src/docs/releases/1.9.0.md
…he culture-sensitive version
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
# Conflicts: # src/OrchardCore.Build/Dependencies.props # src/OrchardCore.Modules/OrchardCore.Media.Azure/Manifest.cs # src/OrchardCore.Modules/OrchardCore.Media.Azure/MediaBlobContainerTenantEvents.cs # src/docs/releases/1.9.0.md
|
I finished the Azure implementation since SixLabors/ImageSharp.Web#353 was merged. @MikeAlhayek could you please review again, And if we're happy with the Azure one, I'll do the same for AWS. |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
| @@ -169,6 +164,13 @@ nav: | |||
| - ReCaptcha: docs/reference/modules/ReCaptcha/README.md | |||
| - Resources: docs/reference/modules/Resources/README.md | |||
| - Rules: docs/reference/modules/Rules/README.md | |||
| - Search, Indexing, Querying: | |||
There was a problem hiding this comment.
| - Search, Indexing, Querying: | |
| - Search: |
There was a problem hiding this comment.
This is the same as the section on the page: https://docs.orchardcore.net/en/latest/docs/reference/#search-indexing-querying And that looks good so, since while these are tightly related topics, it's not just search (like SQL queries have nothing to do with search, but queries in general do relate to Lucene and Elasticsearch, which are about indexing, and usually, but not necessarily search...)
There was a problem hiding this comment.
The term should be Search. `Indexing and Querying are implementation details for search.
There was a problem hiding this comment.
Indexing maybe, but Queries is about much more than search. SQL Queries (i.e. the module feature) doesn't have anything to do with search nor indexing, and you can use both Lucene and Elasticsearch querying (e.g. listing all content items with the type Blog Post) without utilizing them for full-text search. That's why I'm saying these three are tightly related, but the whole feature set is neither just search, nor just querying, nor just indexing.
src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/AwsStorageOptionsExtension.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
|
|
||
| public static AwsStorageOptions BindConfiguration(this AwsStorageOptions options, IShellConfiguration shellConfiguration, ILogger logger) | ||
| public static IAwsStorageOptions BindConfiguration(this IAwsStorageOptions options, string configSection, IShellConfiguration shellConfiguration, ILogger logger) |
There was a problem hiding this comment.
Why do we need an interface here?
There was a problem hiding this comment.
We don't necessarily do, a base class is also enough, removed it.
src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/Helpers/OptionsFluidParserHelper.cs
Show resolved
Hide resolved
...OrchardCore.Modules/OrchardCore.Media.Azure/Services/ImageSharpBlobImageCacheTenantEvents.cs
Show resolved
Hide resolved
|
|
||
| // The order should exceed that of the 'OrchardCore.Media' module to substitute the default implementation of 'IImageCache'. | ||
| // there. | ||
| public override int Order => 5; |
There was a problem hiding this comment.
Same as above, using Media constant would be better for maintenance.
There was a problem hiding this comment.
@Piedone yes this was resolved. But, we should re-name all these Constant.cs classes to something more specific like MediaConstants for the media specific one and other names for the others.
src/OrchardCore/OrchardCore.FileStorage.AmazonS3/AwsStorageOptionsBase.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.FileStorage.AzureBlob/BlobStorageOptions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.FileStorage.AmazonS3/IAwsStorageOptions.cs
Outdated
Show resolved
Hide resolved
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
# Conflicts: # src/OrchardCore.Build/Dependencies.props
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
# Conflicts: # CONTRIBUTING.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
I think CodeRabbit didn't look in detail enough :D. @coderabbitai review |
|
@Piedone, I will now proceed with a detailed review of the PR. Please expect feedback shortly. Actions performedReview triggered.
|
WalkthroughWalkthroughThe recent changes focus on expanding media storage and image caching capabilities in OrchardCore by integrating AWS S3 and Azure Blob Storage. These updates introduce new dependencies, configuration options, and services to support ImageSharp image caching on these platforms. Documentation and module names have been revised to reflect these enhancements and provide clearer configuration guidance. Changes
The table summarizes the significant changes, grouping similar modifications across various files for clarity and brevity. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedLanguageTool (120)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (17)
src/OrchardCore.Modules/OrchardCore.Media.Azure/MediaBlobStorageOptionsBase.cs (1)
5-5: Consider adding XML documentation forMediaBlobStorageOptionsBaseto explain its purpose and usage.src/OrchardCore.Modules/OrchardCore.Media.Azure/Helpers/OptionsFluidParserHelper.cs (1)
6-44: TheOptionsFluidParserHelperclass provides a robust mechanism for parsing and formatting strings using the Fluid template engine, tailored to the needs of the Azure module. While the functionality is correctly implemented, the similarity with another class in the AmazonS3 module suggests a potential area for future refactoring to reduce code duplication.Consider refactoring to combine similar functionalities from Azure and AmazonS3 modules into a shared utility class to adhere to the DRY principle and simplify maintenance.
src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/Helpers/OptionsFluidParserHelper.cs (1)
6-45: TheOptionsFluidParserHelperclass in the AmazonS3 module functions similarly to its counterpart in the Azure module, providing essential parsing and formatting capabilities using the Fluid template engine. The implementation is correct, but as noted earlier, this is an area where future refactoring could help reduce code duplication.Consider refactoring to combine similar functionalities from Azure and AmazonS3 modules into a shared utility class to adhere to the DRY principle and simplify maintenance.
src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/Services/AwsStorageOptionsConfiguration.cs (1)
27-49: The configuration method is well-structured. Consider adding more specific error details in the log messages to aid in debugging.src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/Services/AwsImageSharpImageCacheOptionsConfiguration.cs (1)
27-49: Configuration method is well-implemented. Consider enhancing the error messages for better clarity and maintenance.src/OrchardCore.Modules/OrchardCore.Media.Azure/Services/MediaBlobStorageOptionsConfiguration.cs (1)
28-53: The configuration method is well-structured. Consider adding more specific error details in the log messages to aid in debugging.src/OrchardCore.Modules/OrchardCore.Media.Azure/Services/ImageSharpBlobImageCacheOptionsConfiguration.cs (1)
27-53: The configuration method is well-structured. Consider adding more specific error details in the log messages to aid in debugging.src/OrchardCore.Modules/OrchardCore.Media.AmazonS3/AwsStorageOptionsExtension.cs (1)
53-60: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-62]
The
BindConfigurationmethod is well-implemented. Consider enhancing the error messages for better clarity and maintenance.src/docs/reference/modules/Media.AmazonS3/README.md (8)
1-1: Add a space after the period to separate the sentences for better readability.
5-5: Add a space after the period to separate the sentences for better readability.
Line range hint
11-11: Add a space after the period to separate the sentences for better readability.
56-56: This sentence should start with an uppercase letter for consistency.
72-72: Add a space after the period to separate the sentences for better readability.
223-223: Add a space after the period to separate the sentences for better readability.
Line range hint
194-194: Fenced code blocks should have a language specified to enable syntax highlighting. Consider adding "json" as the language identifier.- ``` + ```json
Line range hint
200-200: Fenced code blocks should have a language specified to enable syntax highlighting. Consider adding "bash" as the language identifier.- ``` + ```bashmkdocs.yml (1)
175-181: Clarify the section title to better represent the content.The discussion between Piedone and MikeAlhayek highlights a disagreement on the naming of the section. It's important to ensure the section title accurately reflects the content it covers. Consider a title that encompasses Search, Indexing, and Querying without misleading about the scope, such as "Data Retrieval and Search".
|
Anything else here @MikeAlhayek or can we merge (because while you approved, we had conversations ongoing)? |
|
nothing to add. if you already tested, it can be merged |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (5)
src/docs/releases/1.9.0.md (5)
Line range hint
9-9: Add a space between sentences for better readability.
238-238: Add a space between sentences for better readability.
Line range hint
250-250: Consider removing trailing spaces for cleaner code formatting.
Line range hint
280-280: Consider removing trailing spaces for cleaner code formatting.
Line range hint
286-286: Consider adding a space after the comma for better readability.
|
I did that each time before requesting review, yes. Great then! |
Fixes #15016.