Skip to content

Conversation

axies20
Copy link

@axies20 axies20 commented Aug 26, 2025

This PR introduces a new integration project that makes it easier to configure Keycloak to run with a Postgres database in Aspire-based applications.

Summary of changes

Added new project CommunityToolkit.Aspire.Keycloak.Postgres.

Implemented an extension method AddPostgres for IResourceBuilder to configure Keycloak with Postgres database resources:

Sets required environment variables (KC_DB, KC_DB_URL_*, KC_DB_USERNAME, KC_DB_PASSWORD, KC_TRANSACTION_XA_ENABLED).

Includes validation for missing or invalid Postgres server configuration.

Updated solution and package references to align with Aspire 9.4.1 dependencies.

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • New integration
    • Docs are written
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes
  • Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

Other information

Project name normalized to Postgres for correctness and consistency.

Extension method returns IResourceBuilder to allow chaining.

Uses proper IValueExpression instead of ToString() for connection strings.

Ensures JDBC-compatible environment variables for Keycloak.

- Introduced a new project `CommunityToolkit.Aspire.Keycloak.Postgress` for configuring Keycloak with PostgreSQL.
- Added an extension method to simplify Keycloak and Postgres database resource integration.
- Updated solution and package references for Aspire dependencies.
@axies20 axies20 changed the title **Add Keycloak with Postgres integration** Add Keycloak with Postgres integration Aug 26, 2025
@davidfowl
Copy link
Contributor

Did you test this?

- Enhanced parameter descriptions in XML documentation.
- Updated method to support asynchronous operations.
- Introduced additional environment variables for finer-grained database configuration.
- Added cancellation token support for improved task management.
@axies20
Copy link
Author

axies20 commented Aug 26, 2025

I have some problems, i can't access host, can you help me?

- Simplified XML documentation and parameter descriptions.
- Removed exceptions for parent resource validation.
- Replaced manual environment variable setup with connection string parsing using `NpgsqlConnectionStringBuilder`.
- Introduced optional port parameter for database configuration.
@axies20
Copy link
Author

axies20 commented Aug 26, 2025

Now it working

@davidfowl
Copy link
Contributor

<ItemGroup Label="Aspire Packages">
<!-- Aspire packages -->
<PackageVersion Include="Aspire.Hosting" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting" Version="9.4.1" />
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is just done when you were testing, but ensure you roll that back and use the MSBuild variable

Copy link
Author

Choose a reason for hiding this comment

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

I had error
0>CommunityToolkit.Aspire.Keycloak.Postgres.csproj: Error NU1605 : Warning As Error: Detected package downgrade: Aspire.Hosting from 9.4.1 to 9.4.0. Reference the package directly from the project to select a different version.
CommunityToolkit.Aspire.Keycloak.Postgres -> Aspire.Hosting.Keycloak 9.4.1-preview.1.25408.4 -> Aspire.Hosting (>= 9.4.1)
CommunityToolkit.Aspire.Keycloak.Postgres -> Aspire.Hosting (>= 9.4.0)
0>------- Finished building project: CommunityToolkit.Aspire.Keycloak.Postgres. Succeeded: False. Errors: 1. Warnings: 0

Copy link
Member

Choose a reason for hiding this comment

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

The main branch is now updated to 9.4.1 Aspire.

@aaronpowell
Copy link
Member

Once there are tests and samples, we can get started on a review of the work.

@davidfowl
Copy link
Contributor

This implemenaton needs work:

  • You don't need to handle the event to make it work
  • This will only work when running but not when publishing.

See dotnet/aspire#8034 for more details.

Copy link
Member

@Alirexaa Alirexaa left a comment

Choose a reason for hiding this comment

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

The package name should be CommunityToolkit.Aspire.Keycloak.Extensions, like other extension packages.

Copy link
Member

Choose a reason for hiding this comment

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

The package name should be CommunityToolkit.Aspire.Keycloak.Extensions, like other extension packages.

Copy link
Author

@axies20 axies20 Aug 27, 2025

Choose a reason for hiding this comment

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

Can i name it like CommunityToolkit.Aspire.Keycloak.Extensions.Postgres?

Copy link
Author

Choose a reason for hiding this comment

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

I think there may be other providers here.

axies20 and others added 3 commits August 27, 2025 13:47
- Renamed `CommunityToolkit.Aspire.Keycloak.Postgress` to `CommunityToolkit.Aspire.Keycloak.Extensions.Postgres`.
- Refactored methods to improve flexibility, including support for different configurations (e.g., development, credentials).
- Introduced a dedicated test project `CommunityToolkit.Aspire.Keycloak.Extensions.Postgres.Tests`.
- Updated solution, dependencies, and project references accordingly.
@axies20
Copy link
Author

axies20 commented Aug 27, 2025

I updated, add event as dev and add parameters for production

/// <returns>
/// The updated resource builder for the Keycloak resource.
/// </returns>
private static void WithPostgres(this IResourceBuilder<KeycloakResource> builder,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will work with dev and production.

Copy link
Author

Choose a reason for hiding this comment

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

You mean void? After that, you can't add other methods, so I returned IResourceBuilder

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I mean that this approach using expressions works in both cases and you don’t need 2 implementations for run and publish

/// <returns>
/// The updated resource builder for the Keycloak resource.
/// </returns>
public static IResourceBuilder<KeycloakResource> WithPostgresDev(this IResourceBuilder<KeycloakResource> builder,
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this overload at all.

Copy link
Author

Choose a reason for hiding this comment

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

You said that event will not work in prod, so I thought that this i will leave for the development environment

Copy link
Contributor

Choose a reason for hiding this comment

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

Right so you don’t need this

Copy link
Member

Choose a reason for hiding this comment

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

There can be conditional logic to check for the different modes rather than having to call different methods.

$"jdbc:postgresql://{ep.Property(EndpointProperty.Host)}:" +
$"{ep.Property(EndpointProperty.Port)}/{dbName}");

builder.WithEnvironment("KC_DB", "postgres")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? Why is this hard coded?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if postgres is password protected (which is the default).

Copy link
Author

@axies20 axies20 Aug 27, 2025

Choose a reason for hiding this comment

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

builder.WithEnvironment("KC_DB", "postgres")
You about this? Yeah, it's correct. There are other database vendors, but I'm writing for postgres
All environment variables: https://www.keycloak.org/server/all-config
I wanna make for other vendors such as dev-file (default), dev-mem, mariadb, mssql, mysql, oracle, postgres (all vendors that support)

Copy link
Author

Choose a reason for hiding this comment

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

What happens if postgres is password protected (which is the default).

public static IResourceBuilder<KeycloakResource> WithPostgres(this IResourceBuilder<KeycloakResource> builder,
        IResourceBuilder<PostgresDatabaseResource> database, ParameterResource username, ParameterResource password)
    {
        ArgumentNullException.ThrowIfNull(username);
        ArgumentNullException.ThrowIfNull(password);
        WithPostgres(builder, database);
        builder.WithEnvironment("KC_DB_USERNAME", username)
            .WithEnvironment("KC_DB_PASSWORD", password);
        return builder;
    }

Here I'm setting by parameter that will set up from user, or what you mean?

Copy link
Member

@aaronpowell aaronpowell left a comment

Choose a reason for hiding this comment

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

This is a hosting integration by the looks of it, so the project should reflect that.

Also, let's just make it Keycloak.Extensions so it's not PG specific, as that'll allow for other extensions on Keycloack to be included in here rather than going through a bunch of micro packages.

/// <returns>
/// The updated resource builder for the Keycloak resource.
/// </returns>
public static IResourceBuilder<KeycloakResource> WithPostgresDev(this IResourceBuilder<KeycloakResource> builder,
Copy link
Member

Choose a reason for hiding this comment

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

There can be conditional logic to check for the different modes rather than having to call different methods.

Comment on lines 3 to 8
<PropertyGroup>
<TargetFramework>net9.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<IsPackable>false</IsPackable>
</PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

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

These aren't needed as they are inherited. Check the other test projects and follow their patterns

Comment on lines 12 to 15
<PackageReference Include="coverlet.collector" Version="6.0.2"/>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.12.0"/>
<PackageReference Include="xunit" Version="2.9.2"/>
<PackageReference Include="xunit.runner.visualstudio" Version="2.8.2"/>
Copy link
Member

Choose a reason for hiding this comment

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

These will fail the build as CPM dictates versions

<ItemGroup Label="Aspire Packages">
<!-- Aspire packages -->
<PackageVersion Include="Aspire.Hosting" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting" Version="9.4.1" />
Copy link
Member

Choose a reason for hiding this comment

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

The main branch is now updated to 9.4.1 Aspire.

<PackageVersion Include="Aspire.Hosting.Dapr" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Azure.AppContainers" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Azure.Redis" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Keycloak" Version="9.4.1-preview.1.25408.4" />
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to introduce a MSBuild variable for the preview version of Aspire packages.

<PackageVersion Include="Aspire.Hosting.MongoDB" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.MySql" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.SqlServer" Version="$(AspireVersion)" />
<PackageVersion Include="Moq" Version="4.20.72" />
Copy link
Member

Choose a reason for hiding this comment

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

This should be under the Testing item group

@aaronpowell aaronpowell changed the title Add Keycloak with Postgres integration [WIP] Add Keycloak with Postgres integration Aug 28, 2025
@aaronpowell aaronpowell requested a review from Copilot August 28, 2025 00:50
Copy link
Contributor

@Copilot Copilot AI left a 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 introduces a new Keycloak-PostgreSQL integration that enables configuring Keycloak resources to use PostgreSQL databases within Aspire applications. The integration provides extension methods to set up the necessary JDBC connection strings and environment variables required by Keycloak to connect to PostgreSQL databases.

  • Extension methods for integrating Keycloak with PostgreSQL databases
  • Support for both development and production scenarios with different credential handling approaches
  • Package reference updates to align with Aspire 9.4.1 dependencies

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/CommunityToolkit.Aspire.Keycloak.Extensions.Postgres/KeycloakPostgresExtension.cs Core extension methods for Keycloak-PostgreSQL integration
src/CommunityToolkit.Aspire.Keycloak.Extensions.Postgres/CommunityToolkit.Aspire.Keycloak.Extensions.Postgres.csproj Project file with necessary Aspire package references
tests/CommunityToolkit.Aspire.Keycloak.Extensions.Postgres.Tests/KeycloakExtensionPostgressTests.cs Empty test class placeholder
tests/CommunityToolkit.Aspire.Keycloak.Extensions.Postgres.Tests/CommunityToolkit.Aspire.Keycloak.Extensions.Postgres.Tests.csproj Test project configuration
Directory.Packages.props Updated Aspire package versions and added new dependencies
CommunityToolkit.Aspire.slnx Solution file updated with new projects

Comment on lines 67 to 68
builder.WithEnvironment("KC_DB", "postgres")
.WithEnvironment("KC_DB", "postgres")
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

The environment variable 'KC_DB' is being set twice with the same value 'postgres'. Remove the duplicate line 68.

Suggested change
builder.WithEnvironment("KC_DB", "postgres")
.WithEnvironment("KC_DB", "postgres")

Copilot uses AI. Check for mistakes.

@axies20
Copy link
Author

axies20 commented Aug 28, 2025

 public static IResourceBuilder<KeycloakResource> WithPostgres(this IResourceBuilder<KeycloakResource> builder,
        IDistributedApplicationBuilder appBuilder, bool xaEnabled = false,
        string usernameParameter = "keycloak-username", string databaseName = "keycloak-db",
        string postgrsName = "keycloak-postgres")
    {
        var username = appBuilder.AddParameter(usernameParameter, "keycloak-db-user");
        var pwd = ParameterResourceBuilderExtensions.CreateDefaultPasswordParameter(appBuilder, "pg-pass");
        var password = appBuilder.CreateResourceBuilder(pwd);
        var keycloakPostgres = appBuilder.AddPostgres(postgrsName, username, password);
        var db = keycloakPostgres.AddDatabase(databaseName);

        builder.WithPostgres(db, username, password, xaEnabled)
            .WithReference(db)
            .WaitFor(keycloakPostgres);
        return builder;
    }

What you think about this method for easy dev environment?

@axies20
Copy link
Author

axies20 commented Sep 23, 2025

Since this is a hosting integration, can you correct the library name to reflect that - it should be CommunityToolkit.Aspire.Keycloak.Hosting.Extensions.

However, since I’m integrating a Postgres database with Keycloak, it might be clearer if the name also included the word Postgres.

@aaronpowell
Copy link
Member

Since this is a hosting integration, can you correct the library name to reflect that - it should be CommunityToolkit.Aspire.Keycloak.Hosting.Extensions.

However, since I’m integrating a Postgres database with Keycloak, it might be clearer if the name also included the word Postgres.

I think it's best we keep it as generic extensions, so if there are other things people want to add for extending Keycloak in the future, we don't end up with a heap of micro packages.

@axies20
Copy link
Author

axies20 commented Sep 27, 2025

Since this is a hosting integration, can you correct the library name to reflect that - it should be CommunityToolkit.Aspire.Keycloak.Hosting.Extensions.

However, since I’m integrating a Postgres database with Keycloak, it might be clearer if the name also included the word Postgres.

I think it's best we keep it as generic extensions, so if there are other things people want to add for extending Keycloak in the future, we don't end up with a heap of micro packages.

Okay

- Updated namespace and project names from `CommunityToolkit.Aspire.Keycloak.Extensions` to `CommunityToolkit.Aspire.Keycloak.Hosting.Extensions`.
- Adjusted all associated file paths, project references, and solution entries.
- Renamed example projects under `examples/keycloak-postgres` to align with the namespace changes.
- Verified updates in integration and unit test coverage.
@axies20 axies20 requested a review from aaronpowell September 28, 2025 13:42
@github-actions github-actions bot added the Stale label Oct 7, 2025
@github-actions github-actions bot closed this Oct 9, 2025
@axies20
Copy link
Author

axies20 commented Oct 10, 2025

@davidfowl Will it be implemented?

@Alirexaa Alirexaa reopened this Oct 13, 2025
@github-actions github-actions bot removed the Stale label Oct 14, 2025
@axies20
Copy link
Author

axies20 commented Oct 15, 2025

@aaronpowell
Is everything alright?

@aaronpowell
Copy link
Member

The integration is still incorrectly named and the namespace doesn't match the contributing guide

- Updated namespaces, project names, and file paths from `CommunityToolkit.Aspire.Keycloak.Extensions` to `CommunityToolkit.Aspire.Keycloak.Hosting.Extensions`.
- Adjusted test projects, README files, and integration tests accordingly.
- Verified alignment with example projects and ensured test coverage.
- Refactored namespaces to align with `Aspire.Hosting`.
- Simplified parameter handling in `WithPostgres` methods.
- Updated README with adjusted examples and clarified usage notes.
- Enhanced project metadata and descriptions for consistency.
- Added missing project references in test projects to ensure coverage.
@axies20 axies20 requested review from Copilot October 20, 2025 09:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.

Comment on lines 56 to 66
var usernameParameter = database.Resource.Parent.UserNameParameter is null
? ReferenceExpression.Create($"postgres")
: ReferenceExpression.Create($"{database.Resource.Parent.UserNameParameter}");

var passwordParameter = database.Resource.Parent.PasswordParameter is null
? ReferenceExpression.Create($"postgres")
: ReferenceExpression.Create($"{database.Resource.Parent.PasswordParameter}");

return WithPostgresData(builder, database, xaEnabled)
.WithEnvironment("KC_DB_USERNAME", usernameParameter)
.WithEnvironment("KC_DB_PASSWORD", passwordParameter);
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

When server parameters are present, this uses string interpolation on the parameter objects, which will yield their type name (via ToString()) instead of a value expression. As a result, KC_DB_USERNAME and KC_DB_PASSWORD will not resolve to the parameter values. Use the ParameterResource from the builder instead. Example fix:

56:         var userParam = database.Resource.Parent.UserNameParameter;
57:         var passParam = database.Resource.Parent.PasswordParameter;
59:
60:         return WithPostgresData(builder, database, xaEnabled)
61:             .WithEnvironment("KC_DB_USERNAME", userParam is not null ? userParam.Resource : ReferenceExpression.Create("postgres"))
62:             .WithEnvironment("KC_DB_PASSWORD", passParam is not null ? passParam.Resource : ReferenceExpression.Create("postgres"));
Suggested change
var usernameParameter = database.Resource.Parent.UserNameParameter is null
? ReferenceExpression.Create($"postgres")
: ReferenceExpression.Create($"{database.Resource.Parent.UserNameParameter}");
var passwordParameter = database.Resource.Parent.PasswordParameter is null
? ReferenceExpression.Create($"postgres")
: ReferenceExpression.Create($"{database.Resource.Parent.PasswordParameter}");
return WithPostgresData(builder, database, xaEnabled)
.WithEnvironment("KC_DB_USERNAME", usernameParameter)
.WithEnvironment("KC_DB_PASSWORD", passwordParameter);
var userParam = database.Resource.Parent.UserNameParameter;
var passParam = database.Resource.Parent.PasswordParameter;
return WithPostgresData(builder, database, xaEnabled)
.WithEnvironment("KC_DB_USERNAME", userParam is not null ? userParam.Resource : ReferenceExpression.Create("postgres"))
.WithEnvironment("KC_DB_PASSWORD", passParam is not null ? passParam.Resource : ReferenceExpression.Create("postgres"));

Copilot uses AI. Check for mistakes.

/// <summary>
/// Provides extension methods for integrating Keycloak resources with PostgreSQL.
/// </summary>
public static class KeycloakPostgresExtension
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

Extension method containers should be suffixed with 'Extensions' to match the project's convention and standard .NET naming. Rename the class to KeycloakPostgresExtensions.

Copilot generated this review using guidance from repository custom instructions.

<TargetFramework>net9.0</TargetFramework>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<TargetFrameworks />
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

An empty TargetFrameworks element overrides TargetFramework and can break restore/build. Remove the empty TargetFrameworks node and keep only TargetFramework.

Suggested change
<TargetFrameworks />

Copilot uses AI. Check for mistakes.

<TargetFramework>net9.0</TargetFramework>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<TargetFrameworks />
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

An empty TargetFrameworks element overrides TargetFramework and can break restore/build. Remove this line and keep only TargetFramework.

Suggested change
<TargetFrameworks />

Copilot uses AI. Check for mistakes.

Comment on lines 33 to 35
public static IResourceBuilder<KeycloakResource> WithPostgres(this IResourceBuilder<KeycloakResource> builder,
IResourceBuilder<PostgresDatabaseResource> database, IResourceBuilder<ParameterResource> username,
IResourceBuilder<ParameterResource> password, bool xaEnabled = false)
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

[nitpick] Public APIs require complete XML documentation. Please add tags describing builder, database, username, password, and xaEnabled so consumers understand expected inputs and behavior.

Copilot uses AI. Check for mistakes.

Comment on lines 48 to 51
public static IResourceBuilder<KeycloakResource> WithPostgres(
this IResourceBuilder<KeycloakResource> builder,
IResourceBuilder<PostgresDatabaseResource> database,
bool xaEnabled = false)
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

[nitpick] Public APIs require complete XML documentation. Please add tags describing builder, database, username, password, and xaEnabled so consumers understand expected inputs and behavior.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

- Extended and clarified XML documentation for `WithPostgres` methods to improve developer understanding.
- Removed unused `<TargetFrameworks />` from example project files.
- Corrected project references in `AppHost` to align with the updated namespace and file paths.
- Cleaned up unused `using` directives in `AppHost.cs`.
Copy link
Member

@aaronpowell aaronpowell left a comment

Choose a reason for hiding this comment

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

Can you please follow the naming conventions from the contributing guide. Hosting integrations should be CommunityToolkit.Aspire.Hosting.<integration name parts>.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be pluralised so it is consistent with all other projects.

Comment on lines 10 to 12
private static IResourceBuilder<KeycloakResource> WithPostgresData(
this IResourceBuilder<KeycloakResource> builder,
IResourceBuilder<PostgresDatabaseResource> database, bool xaEnabled = false)
Copy link
Member

Choose a reason for hiding this comment

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

If parameters are going to be wrapped, can you wrap all for readability.

Comment on lines 41 to 42
IResourceBuilder<PostgresDatabaseResource> database, IResourceBuilder<ParameterResource> username,
IResourceBuilder<ParameterResource> password, bool xaEnabled = false)
Copy link
Member

Choose a reason for hiding this comment

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

If parameters are going to be wrapped, can you wrap all for readability.

/// <param name="password">The builder for the parameter resource representing the password for PostgreSQL authentication.</param>
/// <param name="xaEnabled">Indicates whether XA transactions are enabled for PostgreSQL. Defaults to false.</param>
/// <returns>The updated Keycloak resource builder configured with PostgreSQL integration and explicit credentials.</returns>
public static IResourceBuilder<KeycloakResource> WithPostgres(this IResourceBuilder<KeycloakResource> builder,
Copy link
Member

Choose a reason for hiding this comment

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

If the username and password are made nullable, then the two methods can be combined, with the values using the order of precedence of username -> database.Username -> created in method.

<Project>
<PropertyGroup>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
<AspirePreviewVersion>9.4.1-preview.1.25408.4</AspirePreviewVersion>
Copy link
Member

Choose a reason for hiding this comment

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

This belongs in the Directory.Build.props file, and it should be just the suffix, so that when used we would do <PackageVersion Include="Aspire.Hosting.Keycloak" Version="$(AspireVersion)-$(AspirePreviewSuffix)" />, which ensures that we sync the Major.Minor.Patch across all packages.

<PropertyGroup>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
<AspirePreviewVersion>9.4.1-preview.1.25408.4</AspirePreviewVersion>
<MoqVersion>4.20.72</MoqVersion>
Copy link
Member

Choose a reason for hiding this comment

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

This belongs in Directory.Build.props

<PackageVersion Include="Aspire.Hosting.SqlServer" Version="$(AspireVersion)" />
<PackageVersion Include="Aspire.Hosting.Keycloak" Version="$(AspirePreviewVersion)" />
<PackageVersion Include="Aspire.Keycloak.Authentication" Version="$(AspirePreviewVersion)" />
<PackageVersion Include="Moq" Version="$(MoqVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

This belongs in the <ItemGroup Label="Testing"> section.

@axies20
Copy link
Author

axies20 commented Oct 21, 2025

Since this is a hosting integration, can you correct the library name to reflect that - it should be CommunityToolkit.Aspire.Keycloak.Hosting.Extensions.

I followed your recommendation, okay, I will renamed

- Updated namespaces, project names, and file paths to align with `Aspire.Hosting` naming convention.
- Renamed example projects in `examples/keycloak-postgres` to reflect changes.
- Corrected project references, solution entries, and adjusted test projects accordingly.
- Simplified `WithPostgres` methods with optional username and password handling.
@axies20 axies20 requested a review from aaronpowell October 21, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting response Waiting for the author of the issue to provide more information or answer a question

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants