Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ public static IResourceBuilder<AzureSqlDatabaseResource> AddDatabase(this IResou
/// </summary>
/// <param name="builder">The builder for the Azure SQL resource.</param>
/// <param name="configureContainer">Callback that exposes underlying container to allow for customization.</param>
/// <param name="password">The parameter used to provide the administrator password for the SQL Server resource. If <see langword="null"/> a random password will be generated.</param>
/// <param name="port">The host port for the SQL Server.</param>
/// <returns>A reference to the <see cref="IResourceBuilder{AzureSqlServerResource}"/> builder.</returns>
/// <example>
/// The following example creates an Azure SQL Database (server) resource that runs locally in a
Expand All @@ -146,7 +148,7 @@ public static IResourceBuilder<AzureSqlDatabaseResource> AddDatabase(this IResou
/// builder.Build().Run();
/// </code>
/// </example>
public static IResourceBuilder<AzureSqlServerResource> RunAsContainer(this IResourceBuilder<AzureSqlServerResource> builder, Action<IResourceBuilder<SqlServerServerResource>>? configureContainer = null)
public static IResourceBuilder<AzureSqlServerResource> RunAsContainer(this IResourceBuilder<AzureSqlServerResource> builder, Action<IResourceBuilder<SqlServerServerResource>>? configureContainer = null, IResourceBuilder<ParameterResource>? password = null, int? port = null)
Copy link
Member

@eerhardt eerhardt Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@eerhardt eerhardt Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@DamianEdwards DamianEdwards Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eerhardt eerhardt Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@DamianEdwards DamianEdwards Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member

@davidfowl davidfowl Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eerhardt eerhardt Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@dansiegel dansiegel Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eerhardt eerhardt Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if (builder.ApplicationBuilder.ExecutionContext.IsPublishMode)
{
Expand All @@ -161,7 +163,7 @@ public static IResourceBuilder<AzureSqlServerResource> RunAsContainer(this IReso

RemoveAzureResources(builder.ApplicationBuilder, azureResource, azureDatabases);

var sqlContainer = builder.ApplicationBuilder.AddSqlServer(azureResource.Name);
var sqlContainer = builder.ApplicationBuilder.AddSqlServer(azureResource.Name, password, port);

azureResource.SetInnerResource(sqlContainer.Resource);

Expand Down
2 changes: 1 addition & 1 deletion src/Aspire.Hosting.Azure.Sql/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@ Aspire.Hosting.Azure.AzureSqlServerResource.Databases.get -> System.Collections.
override Aspire.Hosting.Azure.AzureSqlDatabaseResource.Annotations.get -> Aspire.Hosting.ApplicationModel.ResourceAnnotationCollection!
static Aspire.Hosting.AzureSqlExtensions.AddAzureSqlServer(this Aspire.Hosting.IDistributedApplicationBuilder! builder, string! name) -> Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.Azure.AzureSqlServerResource!>!
static Aspire.Hosting.AzureSqlExtensions.AddDatabase(this Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.Azure.AzureSqlServerResource!>! builder, string! name, string? databaseName = null) -> Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.Azure.AzureSqlDatabaseResource!>!
static Aspire.Hosting.AzureSqlExtensions.RunAsContainer(this Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.Azure.AzureSqlServerResource!>! builder, System.Action<Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.SqlServerServerResource!>!>? configureContainer = null) -> Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.Azure.AzureSqlServerResource!>!
static Aspire.Hosting.AzureSqlExtensions.RunAsContainer(this Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.Azure.AzureSqlServerResource!>! builder, System.Action<Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.SqlServerServerResource!>!>? configureContainer = null, Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.ParameterResource!>? password = null, int? port = null) -> Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.Azure.AzureSqlServerResource!>!
Loading