Add MailDev and MailKit custom integration sample - #1891
Add MailDev and MailKit custom integration sample#1891David Pine (IEvangelist) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Aspire sample (samples/maildev-mailkit) demonstrating a custom MailDev hosting integration exported via the Aspire Type System (ATS) and a MailKit client integration consumed by a newsletter API, with both TypeScript and C# AppHost options.
Changes:
- Introduces MailDev hosting integration (
AddMailDev,MailDevResource) and a MailKit client integration with health checks, tracing, and metrics. - Adds a runnable TypeScript AppHost plus a compile-validated C# AppHost equivalent, along with sample newsletter endpoints.
- Adds focused unit tests for the hosting/client integrations and documents setup/run/test steps.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| samples/maildev-mailkit/tsconfig.apphost.json | TypeScript compiler config for the sample AppHost build output. |
| samples/maildev-mailkit/ServiceDefaults/ServiceDefaults.csproj | New ServiceDefaults project to share health check + OpenTelemetry wiring. |
| samples/maildev-mailkit/ServiceDefaults/Extensions.cs | Implements ServiceDefaults extensions and endpoint mapping. |
| samples/maildev-mailkit/README.md | Documentation for ATS exports, credentials flow, running and testing the sample. |
| samples/maildev-mailkit/package.json | Node package scripts/deps for building and linting the TypeScript AppHost. |
| samples/maildev-mailkit/package-lock.json | Locks Node dependencies for reproducible installs. |
| samples/maildev-mailkit/NewsletterService/Properties/launchSettings.json | Local run profiles for the newsletter API. |
| samples/maildev-mailkit/NewsletterService/Program.cs | Newsletter subscribe/unsubscribe endpoints sending SMTP mail via MailKit integration. |
| samples/maildev-mailkit/NewsletterService/NewsletterService.csproj | Newsletter API project wiring (OpenAPI/Scalar + project references). |
| samples/maildev-mailkit/MailKit.Client/Properties/AssemblyInfo.cs | Exposes internals to MailKit client tests. |
| samples/maildev-mailkit/MailKit.Client/MailKitHealthCheck.cs | Health check that validates SMTP connectivity via the factory. |
| samples/maildev-mailkit/MailKit.Client/MailKitExtensions.cs | Builder extensions registering MailKit factory + OTel + health checks. |
| samples/maildev-mailkit/MailKit.Client/MailKitClientSettings.cs | Connection string parsing and configuration settings for SMTP. |
| samples/maildev-mailkit/MailKit.Client/MailKitClientFactory.cs | Scoped SMTP client factory with connection caching per scope. |
| samples/maildev-mailkit/MailKit.Client/MailKit.Client.csproj | MailKit client integration package references/TFM. |
| samples/maildev-mailkit/MailKit.Client.Tests/MailKitExtensionsTests.cs | Unit tests for registration behavior (scoped/keyed/validation). |
| samples/maildev-mailkit/MailKit.Client.Tests/MailKitClientSettingsTests.cs | Unit tests for connection string parsing behavior. |
| samples/maildev-mailkit/MailKit.Client.Tests/MailKit.Client.Tests.csproj | Test project setup for MailKit client integration. |
| samples/maildev-mailkit/MailDev.Hosting/MailDevResourceBuilderExtensions.cs | Adds the MailDev container resource and exports it for ATS. |
| samples/maildev-mailkit/MailDev.Hosting/MailDevResource.cs | Defines MailDev resource model + deferred connection string expression. |
| samples/maildev-mailkit/MailDev.Hosting/MailDev.Hosting.csproj | Hosting integration project for MailDev (Aspire.Hosting dependency). |
| samples/maildev-mailkit/MailDev.Hosting.Tests/MailDevResourceTests.cs | Unit tests validating container config, ATS exports, and connection string expression. |
| samples/maildev-mailkit/MailDev.Hosting.Tests/MailDev.Hosting.Tests.csproj | Test project setup for MailDev hosting integration. |
| samples/maildev-mailkit/eslint.config.mjs | ESLint configuration for the TypeScript AppHost file. |
| samples/maildev-mailkit/CSharpAppHost/Program.cs | C# equivalent AppHost wiring MailDev + newsletter service. |
| samples/maildev-mailkit/CSharpAppHost/CSharpAppHost.csproj | C# AppHost project setup referencing Aspire.Hosting + MailDev.Hosting. |
| samples/maildev-mailkit/aspire.config.json | Configures the TypeScript AppHost and ATS package export for restore. |
| samples/maildev-mailkit/apphost.mts | Runnable TypeScript AppHost consuming ATS-generated APIs. |
| samples/maildev-mailkit/.gitignore | Ignores .aspire/ and build outputs for the sample. |
| README.md | Adds the new sample entry to the repository’s samples table. |
Files not reviewed (1)
- samples/maildev-mailkit/package-lock.json: Generated file
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (!settings.DisableHealthChecks) | ||
| { | ||
| builder.Services.AddHealthChecks().Add(new HealthCheckRegistration( | ||
| serviceKey is null ? "MailKit" : $"MailKit_{connectionName}", |
There was a problem hiding this comment.
Thanks for fixing the duplicate names. I think this is only half fixed: both unkeyed checks still call GetRequiredService<MailKitClientFactory>(), so the last registration wins. A focused test resolves both MailKit_primary and MailKit_secondary to smtp://localhost:1026/. Could we bind each check to its connection and assert the endpoint here, rather than just the label?
6a6bbcd to
ad93e11
Compare
ad93e11 to
f42053e
Compare
| builder, $"{name}-password"); | ||
| var resource = new MailDevResource(name, username?.Resource, passwordParameter); | ||
|
|
||
| return builder.AddResource(resource) |
There was a problem hiding this comment.
Since this sample is marked run-only, could we exclude MailDev from the manifest? I confirmed the pinned head publishes it as container.v0 with its SMTP credential parameter, which teaches the wrong lifecycle for a development mail sink. .ExcludeFromManifest() plus a manifest assertion should cover it.
| .WithHttpEndpoint( | ||
| targetPort: 1080, | ||
| port: httpPort, | ||
| name: MailDevResource.HttpEndpointName) |
There was a problem hiding this comment.
WaitFor(maildev) only waits for the container to reach Running here because the resource has no health check. MailDev exposes /healthz; could we add an HTTP health check so the newsletter service waits for actual readiness rather than racing SMTP startup?
| @@ -0,0 +1,20 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk.Web"> | |||
There was a problem hiding this comment.
Could we add a .slnx for this sample containing the workload, integrations, AppHost, and tests? Repo CI discovers solutions or AppHosts, and this TypeScript sample path only restores ATS + runs npm run aspire:build; NewsletterService, ServiceDefaults, MailKit.Client, the C# AppHost, and the test projects are otherwise not compiled/tested by CI.
| { | ||
| if (settings.Endpoint is null) | ||
| { | ||
| throw new InvalidOperationException("The MailKit SMTP endpoint is not configured."); |
There was a problem hiding this comment.
Could we avoid throwing while constructing the health check dependency? Running NewsletterService without ConnectionStrings:maildev, GET /health returns an unhandled 500 from this line before MailKitHealthCheck.CheckHealthAsync can convert it to Unhealthy. The existing endpoint validation in GetSmtpClientAsync lets the health check report correctly.
| { | ||
| var message = new MimeMessage(); | ||
| message.From.Add(new MailboxAddress("Aspire Newsletter", "newsletter@example.com")); | ||
| message.To.Add(MailboxAddress.Parse(email)); |
There was a problem hiding this comment.
Could we validate with MailboxAddress.TryParse and return 400? I sent {"email":"bad@@example.com"} against the pinned service and this throws MimeKit.ParseException, returning an empty 500; {} and an empty value do the same.
| /// <inheritdoc /> | ||
| public ReferenceExpression ConnectionStringExpression => | ||
| ReferenceExpression.Create( | ||
| $"Endpoint=smtp://{SmtpEndpoint.Property(EndpointProperty.HostAndPort)};Username={UsernameReference};Password={PasswordParameter}"); |
There was a problem hiding this comment.
Since this is an integration-authoring sample, could we implement GetConnectionProperties() for Host, Port, Username, Password, and Uri? The current default is empty, so generated/polyglot consumers only see the opaque connection string and miss the structured pattern we are trying to teach.
| <TargetFramework>net10.0</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| <EnableAspireIntegrationAnalyzers>true</EnableAspireIntegrationAnalyzers> |
There was a problem hiding this comment.
These XML comments do not reach the generated ATS SDK because this project does not emit MailDev.Hosting.xml (confirmed in the build output). Could we enable GenerateDocumentationFile and inspect or assert the generated .d.ts JSDoc?
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
Reviewed pinned head 7ff86d2d6fd5ba479889cfa5a789f641ba434596. The overall shape is sound and these are fixable in place, so I am approving. I think the must-fix items are keeping MailDev run-only, adding real readiness, making repo CI compile/test the .NET projects, and returning Unhealthy instead of an unhandled 500 for missing MailKit configuration. I also left comments on request validation, connection properties, and ATS docs.
The PR head advanced while this review ran; this decision is intentionally attached to the requested pinned commit.
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
Incremental review of f42053e: the MailKit naming fix is incomplete, so I reopened that thread with a failing focused test. Ubuntu is failing in the unrelated Angular sample (typescript@7.0.2 vs @angular-devkit/build-angular requiring <6.1), which #1866 already fixed on main. This head is still conflicting with main and red, so I’m leaving a comment rather than approving. Please merge main, address the MailKit thread, and rerun CI.
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
Approving this as the remaining issue is fixable in place. The health-check thread is still must-fix before merge, and this branch needs updating from main to clear the unrelated Angular failure and workflow conflict.
Summary
Validation