-
Notifications
You must be signed in to change notification settings - Fork 773
Create database for Postgres resource #8085
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
Conversation
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 adds support for database creation in the Postgres container resource, including the ability to run a custom SQL creation script. Key changes include updating test cases to validate database creation scenarios, enhancing the builder extension logic for Postgres and database resources, and updating container image tags for PgAdmin.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.PostgreSQL.Tests/PostgresFunctionalTests.cs | Updated test cases removing redundant environment settings and adding new scenarios for custom scripts, special names, resiliency, and multi-database creation. |
| src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs | Refactored the AddPostgres and AddDatabase logic to handle connection string subscriptions and database creation through a dedicated CreateDatabaseAsync method. |
| src/Aspire.Hosting.PostgreSQL/PostgresDatabaseResource.cs | Revised the ConnectionStringExpression to use a DbConnectionStringBuilder for constructing the connection string. |
| src/Aspire.Hosting.PostgreSQL/PostgresContainerImageTags.cs | Updated the PgAdmin container image tag to a newer version. |
| playground/PostgresEndToEnd/PostgresEndToEnd.AppHost/Program.cs | Adjusted the pgAdmin builder usage by removing an explicit image tag override. |
Comments suppressed due to low confidence (3)
src/Aspire.Hosting.PostgreSQL/PostgresDatabaseResource.cs:36
- Ensure that the connection string constructed via DbConnectionStringBuilder.ToString() produces the expected format with all required parameters for consumers. Validate that no essential connection properties are omitted.
return ReferenceExpression.Create($"{Parent};{connectionStringBuilder.ToString()}");
src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs:589
- When executing a user-provided creation script, consider propagating the exception after logging instead of only logging the error, to avoid silent failures in database creation.
command.CommandText = scriptAnnotation?.Script;
playground/PostgresEndToEnd/PostgresEndToEnd.AppHost/Program.cs:16
- [nitpick] Removing the explicit call to WithImageTag may lead to an unintended pgAdmin image version being deployed; verify that the desired image version is correctly set via configuration or defaults to ensure compatibility.
var pg6 = builder.AddPostgres("pg6").WithPgAdmin(c => c.WithHostPort(8999)).PublishAsContainer();
| command.CommandText = scriptAnnotation?.Script; | ||
| await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (Exception e) |
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.
Don't we want to handle catch (PostgresException p) when (p.SqlState == "42P04") here too? Or is the expectation that they are already checking for database existence.
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.
Since the script could contain other things than create database I didn't want to hide errors. But now it's only supporting create db so it's doing the same thing.
| var connectionStringBuilder = new DbConnectionStringBuilder | ||
| { | ||
| ["Database"] = DatabaseName | ||
| }; |
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.
What's special about this that we need to use the builder? Does it escape properly?
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.
It does, and there is a functional test to validate we can pass database name with special chars to ensure the connection string and the default CREATE DATABASE script are correctly escaping.
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 did it for Sql Server too btw.
Description
Support database creation for
AddDatabase()in Postgres container resource. If the database already exists nothing is done. Custom scripts can be provided.Updated PgAdmin container version.
Part of #7101
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):