Skip to content

Support arbitrary IValueProvider sources in Docker Compose .env file generation#17471

Open
sliekens wants to merge 4 commits into
microsoft:mainfrom
sliekens:fix/docker-compose-env-ivalueprovider-fallback
Open

Support arbitrary IValueProvider sources in Docker Compose .env file generation#17471
sliekens wants to merge 4 commits into
microsoft:mainfrom
sliekens:fix/docker-compose-env-ivalueprovider-fallback

Conversation

@sliekens
Copy link
Copy Markdown
Contributor

Description

When generating the .env file adjacent to the Docker Compose file, DockerComposeEnvironmentResource.PrepareAsync resolved values for two specific source types: ParameterResource and ContainerImageReference. Any other type that implements IValueProvider — including custom resource types defined in external repos — was silently ignored, leaving the env var with whatever static default was set (or blank).

This change collapses the ContainerImageReference-specific branch into a generic IValueProvider branch, and adds a comment explaining the asymmetric treatment of ParameterResource.

-            if (defaultValue is null && envVar.Source is ParameterResource parameter)
+            // Only resolve from the parameter if no static default is already set;
+            // a caller that provides an explicit default intends to skip parameter resolution.
+            if (envVar.Source is ParameterResource parameter)
             {
-                defaultValue = await parameter.GetValueAsync(context.CancellationToken).ConfigureAwait(false);
+                defaultValue ??= await parameter.GetValueAsync(context.CancellationToken).ConfigureAwait(false);
             }
-
-            if (envVar.Source is ContainerImageReference cir)
+            else if (envVar.Source is IValueProvider vp)
             {
-                defaultValue = await ((IValueProvider)cir).GetValueAsync(context.CancellationToken).ConfigureAwait(false);
+                // IValueProvider sources are always resolved dynamically — a static default is never used.
+                defaultValue = await vp.GetValueAsync(context.CancellationToken).ConfigureAwait(false);
             }

ContainerImageReference already implements IValueProvider, so it is handled by the new branch without any change in behavior. ParameterResource is kept as a separate leading branch because it skips resolution when a static default is already set — the else ensures it cannot fall through to the IValueProvider branch.

Alternatives considered

Keep ContainerImageReference explicit and add an IValueProvider fallback as a third branch
The natural first approach is to leave the two existing branches untouched and append a catch-all:

             if (envVar.Source is ContainerImageReference cir)
             {
                 defaultValue = await ((IValueProvider)cir).GetValueAsync(context.CancellationToken).ConfigureAwait(false);
             }
+            else if (envVar.Source is IValueProvider vp)
+            {
+                defaultValue = await vp.GetValueAsync(context.CancellationToken).ConfigureAwait(false);
+            }

This works, but the ContainerImageReference branch becomes dead weight — it does the same thing the fallback would do, and requires a comment explaining why the else is there to prevent double-resolution. Collapsing it into the fallback is strictly simpler.

Collapse both branches into a single IValueProvider check
Removing the ParameterResource branch entirely would produce the shortest code, but it loses the ??= guard: the parameter would always be resolved even when a static default is already set. That is a behavioral regression for existing callers.

-            if (envVar.Source is ParameterResource parameter)
-            {
-                defaultValue ??= await parameter.GetValueAsync(context.CancellationToken).ConfigureAwait(false);
-            }
-            else if (envVar.Source is IValueProvider vp)
+            if (envVar.Source is IValueProvider vp)
             {
                 defaultValue = await vp.GetValueAsync(context.CancellationToken).ConfigureAwait(false);
             }

Change Source from object? to IValueProvider?
A type-safe property would eliminate the runtime pattern-matching entirely, but ContainerMountAnnotation and ContainerPortReference are also valid Source values and do not implement IValueProvider. Moving them to a separate property would be a public API break with no benefit over the current fix.

-    [AspireUnion(typeof(ParameterResource), typeof(ContainerMountAnnotation), typeof(ContainerImageReference), typeof(ContainerPortReference))]
-    public object? Source { get; set; }
+    [AspireUnion(typeof(ParameterResource), typeof(ContainerImageReference))]
+    public IValueProvider? Source { get; set; }

Make ConfigureEnvFile async
The ConfigureEnvFile callback is synchronous, so callers cannot await their own value resolution. Changing its type would let any caller handle arbitrary sources asynchronously — but this pushes resolution responsibility out of the framework and onto every caller, and is a breaking change to the internal API.

-internal Action<IDictionary<string, CapturedEnvironmentVariable>>? ConfigureEnvFile { get; set; }
+internal Func<IDictionary<string, CapturedEnvironmentVariable>, CancellationToken, Task>? ConfigureEnvFile { get; set; }

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
    • No

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2026

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17471

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17471"

@sliekens sliekens marked this pull request as ready for review May 25, 2026 13:13
sliekens and others added 2 commits May 25, 2026 15:39
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread tests/Aspire.Hosting.Docker.Tests/DockerComposePublisherTests.cs Outdated
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.

2 participants