Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple domains via ConfigureCustomDomain #7309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matthebrown
Copy link
Contributor

@matthebrown matthebrown commented Jan 29, 2025

Description

Adds support for multiple domains via ConfigureCustomDomain by appending the domain to the domain list instead of replacing the list entirely.

Fixes #7143

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
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 29, 2025
@davidfowl
Copy link
Member

It feels strange to make Configure* Add maybe it should be AddCustomDomain

@matthebrown
Copy link
Contributor Author

It feels strange to make Configure* Add maybe it should be AddCustomDomain

AddCustomDomain feels more intuitive that calls can be chained to add multiple domains if desired.

@davidfowl
Copy link
Member

We can't rename the method, we need to add an overload.

@davidfowl
Copy link
Member

@matthebrown any plans to update this? This method is in preview so we can break it, there are no guarantees. That said, I'm unsure if we should break it.

Maybe keep the rename and just update the branch lets evaluate if it should be a new method or a behavior change.

@matthebrown
Copy link
Contributor Author

@matthebrown any plans to update this? This method is in preview so we can break it, there are no guarantees. That said, I'm unsure if we should break it.

Maybe keep the rename and just update the branch lets evaluate if it should be a new method or a behavior change.

Sounds good. I'll keep the rename as suggested and we can open discussion here or in the linked issue. Happy to adjust the changes as needed.

@matthebrown matthebrown force-pushed the AddCustomDomains branch 2 times, most recently from 1b42800 to a89b139 Compare February 8, 2025 15:22
@davidfowl
Copy link
Member

davidfowl commented Feb 8, 2025

Maybe the way to do this is to apply dictionary like semantics (add or update), based on the name. We don't rename the method and its not exactly append.

@davidfowl
Copy link
Member

Why did you add @ to all of the strings? Can you undo that change?

@mip1983
Copy link

mip1983 commented Feb 9, 2025

I tried to do something along the lines of what this merge is doing by putting my own extension method, but as soon as I try and add multiple domains, I get this error during provisioning:

ERROR: generating apphost manifest: generating app host manifest: dotnet run --publisher manifest on project 'D:\a\_work\1\s\ecoDriverInfrastructure\ecoDriverInfrastructure.AppHost\ecoDriverInfrastructure.AppHost.csproj' failed: exit code: 3762504530, stdout: Using launch settings from Properties\launchSettings.json...
Building...
info: Aspire.Hosting.DistributedApplication[0]
      Aspire version: 9.0.0+01ed51919f8df692ececce51048a140615dc759d
fail: Microsoft.Extensions.Hosting.Internal.Host[11]
      Hosting failed to start
      System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
         at System.Collections.Generic.List`1.Enumerator.MoveNext()
         at Aspire.Hosting.Publishing.ManifestPublishingContext.WriteModel(DistributedApplicationModel model, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublishingContext.cs:line 75
         at Aspire.Hosting.Publishing.ManifestPublisher.WriteManifestAsync(DistributedApplicationModel model, Utf8JsonWriter jsonWriter, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublisher.cs:line 53
         at Aspire.Hosting.Publishing.ManifestPublisher.PublishInternalAsync(DistributedApplicationModel model, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublisher.cs:line 42
         at Aspire.Hosting.Publishing.ManifestPublisher.PublishAsync(DistributedApplicationModel model, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublisher.cs:line 26
         at Microsoft.Extensions.Hosting.Internal.Host.<StartAsync>b__14_1(IHostedService service, CancellationToken token)
         at Microsoft.Extensions.Hosting.Internal.Host.ForeachService[T](IEnumerable`1 services, CancellationToken token, Boolean concurrent, Boolean abortOnFirstException, List`1 exceptions, Func`3 operation)
, stderr: Unhandled exception. System.AggregateException: One or more errors occurred. (Collection was modified; enumeration operation may not execute.)
 ---> System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.List`1.Enumerator.MoveNext()
   at Aspire.Hosting.Publishing.ManifestPublishingContext.WriteModel(DistributedApplicationModel model, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublishingContext.cs:line 75
   at Aspire.Hosting.Publishing.ManifestPublisher.WriteManifestAsync(DistributedApplicationModel model, Utf8JsonWriter jsonWriter, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublisher.cs:line 53
   at Aspire.Hosting.Publishing.ManifestPublisher.PublishInternalAsync(DistributedApplicationModel model, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublisher.cs:line 42
   at Aspire.Hosting.Publishing.ManifestPublisher.PublishAsync(DistributedApplicationModel model, CancellationToken cancellationToken) in /_/src/Aspire.Hosting/Publishing/ManifestPublisher.cs:line 26
   at Microsoft.Extensions.Hosting.Internal.Host.<StartAsync>b__14_1(IHostedService service, CancellationToken token)
   at Microsoft.Extensions.Hosting.Internal.Host.ForeachService[T](IEnumerable`1 services, CancellationToken token, Boolean concurrent, Boolean abortOnFirstException, List`1 exceptions, Func`3 operation)
   at Microsoft.Extensions.Hosting.Internal.Host.StartAsync(CancellationToken cancellationToken)
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.RunAsync(IHost host, CancellationToken token)
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.RunAsync(IHost host, CancellationToken token)
   at Aspire.Hosting.DistributedApplication.RunAsync(CancellationToken cancellationToken) in /_/src/Aspire.Hosting/DistributedApplication.cs:line 311
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait()
   at Aspire.Hosting.DistributedApplication.Run() in /_/src/Aspire.Hosting/DistributedApplication.cs:line 339
   at Program.<Main>$(String[] args) in D:\a\_work\1\s\ecoDriverInfrastructure\ecoDriverInfrastructure.AppHost\Program.cs:line 251

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConfigureCustomDomain Only Supports One Domain
3 participants