-
Notifications
You must be signed in to change notification settings - Fork 704
Add ability to configure Port and Password for Redis and Postgres and SqlServer when they configured to run by RunAsContainer
#8439
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
Add ability to configure Port and Password for Redis and Postgres and SqlServer when they configured to run by RunAsContainer
#8439
Conversation
….com/alirexaa/aspire into azure-sql-configure-run-as-container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the SQL Server container configuration by adding a new options parameter for specifying the port and password, and extends testing to verify these changes.
- Adds a new parameter, configureOptions, to the RunAsContainer method.
- Introduces the RunAsContainerOptions class to encapsulate port and password settings.
- Adds unit tests to verify that the SQL Server container is properly configured.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/Aspire.Hosting.Azure.Tests/AzureSqlExtensionsTests.cs | Added unit tests to validate password and port configuration for containerized SQL Server |
| src/Aspire.Hosting.Azure.Sql/RunAsContainerOptions.cs | New options class for configuring SQL Server container settings |
| src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs | Extended the RunAsContainer method to support configuration via RunAsContainerOptions |
Files not reviewed (1)
- src/Aspire.Hosting.Azure.Sql/CompatibilitySuppressions.xml: Language not supported
Comments suppressed due to low confidence (1)
src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs:153
- [nitpick] The parameters 'configureContainer' and 'configureOptions' are similar in naming, which could cause confusion. Consider renaming 'configureOptions' to 'configureContainerOptions' for improved clarity.
public static IResourceBuilder<AzureSqlServerResource> RunAsContainer(this IResourceBuilder<AzureSqlServerResource> builder, Action<IResourceBuilder<SqlServerServerResource>>? configureContainer = null, Action<RunAsContainerOptions>? configureOptions = null)
347c77b to
f462b63
Compare
Co-authored-by: Sébastien Ros <[email protected]>
|
@Alirexaa - check out the discussion here: #6886 (comment) Basically, I think a better API approach is: var somePassword = ...;
var sql1 = builder.AddAzureSqlServer("sql1")
.RunAsContainer(c =>
{
c.WithHostPort(5555)
.WithPassword(somePassword);
}); |
|
OK @eerhardt, I will update the PR to reflect your approach. |
|
@eerhardt, Some adjustments are needed, but please take a look. After your feedback, I will add more tests and XML docs. |
RunAsContainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the resource builders for PostgreSQL, Redis, and SQL Server by adding configuration options for host port and password when running as a container. The changes include new extension methods and resource APIs to set passwords and host ports, as well as new and updated tests to assert that these settings are correctly applied.
- Added WithPassword and WithHostPort extension methods for SQL Server, Redis, and PostgreSQL resources.
- Updated resource classes (e.g., SqlServerServerResource, RedisResource, PostgresServerResource) to store password parameters.
- Introduced tests for both containerized Azure and non-Azure resources to verify connection string construction.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.SqlServer.Tests/AddSqlServerTests.cs | New tests for SQL Server host port and password configuration. |
| tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs | New tests to verify Redis host port and password setup. |
| tests/Aspire.Hosting.PostgreSQL.Tests/AddPostgresTests.cs | Added tests for PostgreSQL host port, password, and username configurations. |
| tests/Aspire.Hosting.Azure.Tests/AzureSqlExtensionsTests.cs | Added tests for containerized SQL Server resources. |
| tests/Aspire.Hosting.Azure.Tests/AzureRedisExtensionsTests.cs | New tests for Azure Redis resource run-as-container configuration. |
| tests/Aspire.Hosting.Azure.Tests/AzurePostgresExtensionsTests.cs | New tests for Azure Postgres flexible server settings. |
| src/Aspire.Hosting.SqlServer/SqlServerServerResource.cs | Updated to support password setting with a private setter and new API. |
| src/Aspire.Hosting.SqlServer/SqlServerBuilderExtensions.cs | Added WithPassword and WithHostPort methods for SQL Server resource. |
| src/Aspire.Hosting.Redis/RedisResource.cs | Updated to include a SetPassword method with a private setter. |
| src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs | Added WithPassword and WithHostPort methods for Redis resource. |
| src/Aspire.Hosting.PostgreSQL/PostgresServerResource.cs | Added SetPassword and SetUserName methods to support new configuration. |
| src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs | Added WithPassword, WithUserName, and WithHostPort methods for Postgres. |
Files not reviewed (1)
- .config/CredScanSuppressions.json: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good. Thanks @Alirexaa!
Just a few minor comments and then I think this can be merged.
.config/CredScanSuppressions.json
Outdated
| "_justification": "Legitimate UT certificate file with private key, from dotnet/aspnetcore" | ||
| }, | ||
| { | ||
| "placeholder": "user1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure.
| { | ||
| ArgumentNullException.ThrowIfNull(password); | ||
|
|
||
| PasswordParameter = password; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These properties are publically settable. We can remove these methods and just set the property.
| return builder.WithEnvironment(context => | ||
| { | ||
| context.EnvironmentVariables[PasswordEnvVarName] = builder.Resource.PasswordParameter; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need this because this code grabs the parameters from the resource, so it will continue to work if we set the parmaeter correctly.
aspire/src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs
Lines 110 to 113 in fd413eb
| .WithEnvironment(context => | |
| { | |
| context.EnvironmentVariables[UserEnvVarName] = postgresServer.UserNameReference; | |
| context.EnvironmentVariables[PasswordEnvVarName] = postgresServer.PasswordParameter; |
(repeated elsewhere) #Closed
| var password = "p@ssw0rd1"; | ||
| var pass = builder.AddParameter("pass", password); | ||
| var redis = builder. | ||
| AddRedis("myRedis") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are various code cleanups scattered around. We typically put the . on the trailing line, with the method being invoked.
Along with this, there are unnecessary blank lines and other whitespace issues. Can you clean these up?
Revert unnecessary changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the great work here, @Alirexaa!
Description
This pull request introduces several enhancements and new features for PostgreSQL, Redis, and SQL Server resource builders, along with some test updates. The key changes include adding methods to configure passwords, usernames, and host ports for these resources, as well as updates to the corresponding tests to ensure these configurations are correctly applied.
Enhancements to resource builders:
PostgresBuilderExtensions.cs.RedisBuilderExtensions.cs.SqlServerBuilderExtensions.cs.Internal resource updates:
PostgresServerResource.csto include methods for setting passwords and usernames.RedisResource.csto include a method for setting passwords.SqlServerServerResource.csto include a method for setting passwords.Test updates:
AzurePostgresExtensionsTests.csto verify the correct configuration of usernames and passwords for PostgreSQL resources.AzureRedisExtensionsTests.csto verify the correct configuration of host and password for Redis resources.AzureSqlExtensionsTests.csto include necessary imports for new tests.Miscellaneous:
.config/CredScanSuppressions.json.RedisBuilderExtensions.cs.Fixes #7918
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):