Skip to content
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

WIP: Add Marten Client #60

Closed
wants to merge 17 commits into from
Closed

WIP: Add Marten Client #60

wants to merge 17 commits into from

Conversation

Alirexaa
Copy link
Member

@Alirexaa Alirexaa commented Oct 3, 2024

Closes #58

This pull request includes several changes to the Aspire.CommunityToolkit.sln solution and introduces a new project for Marten integration. The most important changes include updating project GUIDs, adding new package references, and implementing Marten client configuration and health checks.

Solution File Updates:

  • Updated project GUIDs for multiple projects to ensure consistency in Aspire.CommunityToolkit.sln. [1] [2]
  • Added the new Aspire.CommunityToolkit.Marten project to the solution. [1] [2]

Package References:

  • Added AspNetCore.HealthChecks.NpgSql and Marten package references in Directory.Packages.props. [1] [2]

New Project for Marten Integration:

  • Created Aspire.CommunityToolkit.Marten.csproj with necessary package references and file links.
  • Implemented MartenApplicationBuilderExtensions to configure Marten clients and health checks.
  • Added MartenSettings class to manage Marten client configuration settings.

Miscellaneous:

  • Updated DocsPath in Directory.Build.props to point to README.md. [1] [2]
  • Added HealthChecksExtensions to support conditional health check registration.

PR Checklist

  • Based off latest main branch of toolkit
  • 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

one thing we should consider is NpgsqlDataSource registration should not make conflict with other NpgsqlDataSource instance. (for instance NpgsqlDataSource registration in Aspire.NpgSql)

builder.Services.AddKeyedSingleton<NpgsqlDataSource>(serviceKey is null ? "Aspire.Marten" : $"Aspire.Marten_{connectionName}", (serviceProvider, _) =>
        {
            ValidateConnectionString(settings.ConnectionString, connectionName, DefaultConfigSectionName);
            var dataSourceBuilder = new NpgsqlDataSourceBuilder(settings.ConnectionString);
            dataSourceBuilder.UseLoggerFactory(serviceProvider.GetService<ILoggerFactory>());
            return dataSourceBuilder.Build();
        });

Other information

@Alirexaa Alirexaa self-assigned this Oct 3, 2024
@Alirexaa Alirexaa added the integration A new .NET Aspire integration label Oct 3, 2024
@Alirexaa
Copy link
Member Author

Alirexaa commented Oct 3, 2024

@aaronpowell
Copy link
Member

@aaronpowell, for testing clients we should have copy of https://github.com/dotnet/aspire/blob/f2c016d81420aef66c1a9c994b670367d1a3e54c/tests/Aspire.Components.Common.Tests/ConformanceTests.cs

Dang, I just deleted the ConditionalFact and ConditionalTheory types 🤣.

What additional dependencies/types are needed for that base test class?

Aspire.CommunityToolkit.sln Outdated Show resolved Hide resolved
src/HealthChecksExtensions.cs Outdated Show resolved Hide resolved
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.

We need to have an example project that uses this integration, and I think it would make sense to use that within the integration tests rather than having to simulate a client.


namespace CommunityToolkit.Aspire.Marten.Tests;

public class AspireMartenClientExtensionsTest(PostgreSQLContainerFixture containerFixture) : IClassFixture<PostgreSQLContainerFixture>
Copy link
Member

Choose a reason for hiding this comment

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

Would we be better creating an example app host that has a Postgres resource in it that is provided to the client that is using Marten, and then have the tests run that whole stack rather than trying to completely emulate just a client in the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have not enogh tests.
I want to add more funtional tests.
Example project is one of those.

@Alirexaa
Copy link
Member Author

@jeremydmiller, We are adding Marten integration to the aspire via the community toolkit.
I mark this package as experimental because some problems prevent achieving all Marten features like AsyncDaemon.
Please take a look. Any comment will be helpful.

@@ -0,0 +1,3 @@
using System.Diagnostics.CodeAnalysis;

[assembly: Experimental("CTASPIREMARTEN001", UrlFormat = "https://aka.ms/dotnet/aspire/diagnostics#{0}")]
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd be better putting this on the extension methods rather than at the assembly as a whole.

For the link, can you use https://aka.ms/communitytoolkit/aspire/diagnostics#{0} as that'll take them to our docs on what the code means.

Also, to we need to document that code in the docs/diagnostics.md file.

Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
CommunityToolkit.Aspire.Hosting.Azure.StaticWebApps 100% 100% 28
CommunityToolkit.Aspire.Hosting.Deno 84% 75% 72
CommunityToolkit.Aspire.Hosting.Deno.AppHost 100% 100% 2
CommunityToolkit.Aspire.Hosting.Golang 94% 50% 16
CommunityToolkit.Aspire.Hosting.Golang.AppHost 100% 100% 2
CommunityToolkit.Aspire.Hosting.Java 98% 71% 58
CommunityToolkit.Aspire.Hosting.Java.AppHost 50% 100% 2
CommunityToolkit.Aspire.Hosting.Meilisearch 44% 16% 88
CommunityToolkit.Aspire.Hosting.Meilisearch.AppHost 50% 100% 2
CommunityToolkit.Aspire.Hosting.NodeJS.Extensions 90% 68% 92
CommunityToolkit.Aspire.Hosting.NodeJS.Extensions.AppHost 100% 100% 2
CommunityToolkit.Aspire.Hosting.Ollama 52% 32% 154
CommunityToolkit.Aspire.Hosting.Ollama.AppHost 50% 100% 2
CommunityToolkit.Aspire.Marten 100% 91% 80
CommunityToolkit.Aspire.Marten.AppHost 50% 100% 2
CommunityToolkit.Aspire.Meilisearch 97% 92% 68
CommunityToolkit.Aspire.OllamaSharp 92% 80% 30
CommunityToolkit.Aspire.StaticWebApps.AppHost 100% 100% 2
CommunityToolkit.Aspire.Testing 61% 50% 446
Summary 73% (1956 / 2686) 55% (466 / 840) 1148

Minimum allowed line rate is 60%

@jeremydmiller
Copy link

@jeremydmiller, We are adding Marten integration to the aspire via the community toolkit. I mark this package as experimental because some problems prevent achieving all Marten features like AsyncDaemon. Please take a look. Any comment will be helpful.

Oh, guys, I'm sorry, but I'm not enthusiastic about this at all. Marten can happily work with a single connection brought in through the existing Aspire Npgsql integration, and I'd planned to leave it at that. I get that having the recipe to add health checks and Otel wired up would be nice, but I'd rather have that in a package controlled by Marten itself than over to the side where this all will likely break.

I'm admittedly not very enthusiastic about Aspire in general anyway, but I really don't like how this eliminates a lot of important features for Marten and the Marten/Wolverine integration.

@aaronpowell
Copy link
Member

Thanks for taking the time to have a look @jeremydmiller and reply. Given you don't have a desire for an Aspire integration for Marten I'll go ahead and close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration A new .NET Aspire integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue For migrate integration from dotnet/aspire repo
3 participants