-
Notifications
You must be signed in to change notification settings - Fork 41
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
#240 add papercut smtp hosting #423
base: main
Are you sure you want to change the base?
#240 add papercut smtp hosting #423
Conversation
I don't understand why my PR builds seem to fail often. Any ideas on this? |
I found a solution by changing to test against the Papercut Smtp container directly. |
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.
Could you remove the ActiveMQ changes from this PR (create a separate PR for them), and some comments on how to improve the tests and healthchecks
"ConnectionStrings": { | ||
"papercut": "smpt://xyz:12345" | ||
}, |
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.
Should remove this as the integration should add it.
/// ConnectionString for the Papercut SMTP server in the form of smtp://host:port. | ||
/// </summary> | ||
public ReferenceExpression ConnectionStringExpression => ReferenceExpression.Create( | ||
$"smtp://{SmtpEndpoint.Property(EndpointProperty.Host)}:{SmtpEndpoint.Property(EndpointProperty.Port)}"); |
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.
For consistency, let's do Endpoint=smtp...
and then parse it using the DbConnectionStringBuilder
like we do with Ollama
/// <summary> | ||
/// Adds Papercut SMTP to the application model. | ||
/// </summary> | ||
/// <param name="builder">The <see cref="IDistributedApplicationBuilder"/> to add the resource to.</param> | ||
/// <param name="name">The name of the resource.</param> | ||
/// <remarks> | ||
/// </remarks> | ||
/// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns> | ||
public static IResourceBuilder<PapercutSmtpContainerResource> AddPapercutSmtp(this IDistributedApplicationBuilder builder, | ||
[ResourceName] string name) | ||
{ | ||
return builder.AddPapercutSmtp(name, null); | ||
} |
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 can be removed as the other method provides defaults for httpPort
and smtpPort
so you can call it without any argument other than name
.
src/CommunityToolkit.Aspire.Hosting.PapercutSmtp/PapercutSmtpHostingExtension.cs
Outdated
Show resolved
Hide resolved
tests/CommunityToolkit.Aspire.Hosting.PapercutSmtp.Tests/AppHostTests.cs
Outdated
Show resolved
Hide resolved
tests/CommunityToolkit.Aspire.Hosting.PapercutSmtp.Tests/ContainerResourceCreationTests.cs
Outdated
Show resolved
Hide resolved
98675ad
to
803b914
Compare
@aaronpowell thanks for the review |
**Closes #240 **
PR Checklist
Other information