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

#240 add papercut smtp hosting #423

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

anoordover
Copy link
Contributor

**Closes #240 **

PR Checklist

  • 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

@anoordover
Copy link
Contributor Author

I don't understand why my PR builds seem to fail often. Any ideas on this?

@anoordover
Copy link
Contributor Author

I found a solution by changing to test against the Papercut Smtp container directly.

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.

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

Comment on lines 2 to 4
"ConnectionStrings": {
"papercut": "smpt://xyz:12345"
},
Copy link
Member

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)}");
Copy link
Member

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

Comment on lines 11 to 23
/// <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);
}
Copy link
Member

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.

@anoordover anoordover marked this pull request as draft January 30, 2025 07:07
@anoordover anoordover force-pushed the feature/240_Papercut branch from 98675ad to 803b914 Compare January 30, 2025 07:09
@anoordover anoordover marked this pull request as ready for review January 30, 2025 08:23
@anoordover
Copy link
Contributor Author

@aaronpowell thanks for the review

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

Successfully merging this pull request may close these issues.

New Integration: PaperCut
2 participants