Provide optional arguments for password/port when running as a container

[dansiegel]

Description

Adds optional parameters from the underlying call to AddSqlServer for the password and the port. This allows users to specify these properties when using Azure Sql to run locally as a container for development and still optionally configure the password/port like you can when using the SqlServer AddSqlServer method directly.

This can be helpful for local development as the port will shift each time the AppHost is run.

Fixes # (issue)

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
  • Updated existing with new optional parameters
• 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?
  • Yes
    • Is this introducing a breaking change?
      • Yes
        • Link to aspire-docs issue (please use this breaking-change template):
      • No
        • Link to aspire-docs issue (please use this doc-idea template):
  • No

Microsoft Reviewers: Open in CodeFlow

[davidfowl]

@eerhardt we should do this in 9.1

[eerhardt]

@davidfowl @sebastienros @ReubenBond @JamesNK @DamianEdwards - are we concerned with binary breaking changes like this in the Hosting integrations? Part of me thinks this is OK, but wanted to check what everyone else thinks.

If we didn't go with this approach, we could take one of 2 approaches:

1. Just add 1 new overload that took all parameters, and callers would pass in null explicitly for the parameters they aren't supplying.
2. Explode the API out with 8 overloads - every combination of parameters - which feels excessive.

[eerhardt]

An alternative is to expose some methods that allow for configuring the port and the password during the configureContainer callback.

See 15e8a26 for an example:

var sql1 = builder.AddAzureSqlServer("sql1")
                  .RunAsContainer(c =>
                  {
                      c.WithHostPort(5555);
                  });

The downside with just exposing it on these 3 containers is that it isn't consistent with the rest of the "top level" containers - like MongoDB, RabbitMQ, etc. If we go with this route, would we add it to all of them as well?

[DamianEdwards]

I think we've generally been fine with adding new optional parameters to existing methods right?

[eerhardt]

We haven't been fine in the client integration libraries, since it is a binary breaking change. But I'm not sure we care as much in the hosting integrations libraries.

[DamianEdwards]

Yeah I guess I'm basing this on what we've done in ASP.NET Core itself and Aspire.Hosting, or I'm totally misremembering 😄

[davidfowl]

Don't make binary breaking changes in minors. Just add overloads 😄

[eerhardt]

I think I'm in favor of taking the alternative approach above:

var sql1 = builder.AddAzureSqlServer("sql1")
                  .RunAsContainer(c =>
                  {
                      c.WithHostPort(5555);
                  });

This fits with our other "emulator" containers, and allows us to not have an explosion of overloads.

We would also then have a .WithPassword(IResourceBuilder<ParameterResource>) extension as well that allows you to set the password after the constructor.

Opinions on taking this approach?

[dansiegel]

I thought about that as well but it would require more changes to the way that the underlying SqlServerResource is created/updated.

[eerhardt]

I agree it would require some more changes throughout, but I think it provides a better API experience.