umbraco / Umbraco.StorageProviders

MIT License
30 stars 22 forks source link

Add / Configure via composer #56

Closed soreng closed 10 months ago

soreng commented 1 year ago

As in the Umbraco.Cloud version of this, it would be easier to setup by using a composer like this:

https://github.com/umbraco/Umbraco.Cloud.StorageProviders.AzureBlob/commit/350adf070240afcbfb734ffaf74766745536c5ed

The reasoning for this is that when you work locally, you would then need the code to check for environment before calling .AddAzureBlobFileSystem

public void ConfigureServices(IServiceCollection services)
{
    services.AddUmbraco(_env, _config)
        .AddBackOffice()
        .AddWebsite()
        .AddComposers()
+       .AddAzureBlobMediaFileSystem()
        .Build();
}

Would then be (like)

public void ConfigureServices(IServiceCollection services)
{
    var umbraco = services.AddUmbraco(_env, _config)
        .AddBackOffice()
        .AddWebsite()
        .AddComposers();
+        if(!_env.IsEnvironment("local")){
+                umbraco.AddAzureBlobMediaFileSystem()
+        }
+        umbraco.Build();
}

The composer in the Umbraco.Cloud version checks if the configuration is in place, before calling .AddAzureBlobMediaFileSystem()

ronaldbarendse commented 1 year ago

This package deliberately doesn't ship with a composer to avoid 'automagically' adding the services using the default configuration section, resulting in validation errors when the required configuration isn't in place. And conditionally adding these services could potentially also result in a misconfigured environment to start writing media files to the default local PhysicalFileSystem or suddenly return 404s when trying to fetch existing media files. In both cases, you need to be explicit on the behaviour you want when adding the services yourself...

The Umbraco Cloud package only checks environment variables for Cloud-specific configuration values and since we ensure all hosted Cloud projects have these variables set, we can safely assume that you're running a local environment when these variables are not present.

Creating a composer to conditionally add the services when the configuration sections exist isn't too complicated to write yourself either:

using Umbraco.Cms.Core.Composing;

public class StorageProvidersComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        if (builder.Config.GetSection("Umbraco:Storage:Cdn").Exists())
        {
            builder.AddCdnMediaUrlProvider();
        }

        if (builder.Config.GetSection("Umbraco:Storage:AzureBlob:Media").Exists())
        {
            builder.AddAzureBlobMediaFileSystem();
            builder.AddAzureBlobImageSharpCache();
        }
    }
}

I would be okay with adding this example to the documentation, but only if it's accompanied with a warning that this might result in misconfigured environments to silently skip configuration, instead of returning a validation error indicating the missing configuration. And maybe a similar example composer can be created that conditionally adds the services based on environment name (like you showed in the ConfigureServices example)?

ronaldbarendse commented 10 months ago

I've now added an example site in PR https://github.com/umbraco/Umbraco.StorageProviders/pull/61 that uses a composer to add/configure the services and you can otherwise use the example from the above comment to conditionally add it.

The main reasoning is that this type of configuration relies on external infrastructure and shouldn't change based on the absence of a configuration section/variable. Falling back to the default physical media file system could result in hard to debug runtime issues, especially when using load balancing and/or if you have limited local storage.