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

feat: Reintroduce Papercut module #1268

Merged

Conversation

TechLiam
Copy link
Contributor

What does this PR do?

Added a wait strategy using HTTP request is succeeded for the health endpoint for Papercut WebUI Service.
This should stop the failing on the pipeline raised here #1265

Why is it important?

The wait strategy allows the container to be ready for testing and usage when starting up especially in a very busy environment

Related issues

None

How to test this PR

Tests pass now when running dotnet cake
Screenshot 2024-09-20 172804

Follow-ups

Hopefully, there is enough here to stop the issues @HofmeisterAn was having when doing other modules

Copy link

netlify bot commented Sep 20, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit eb05e63
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/670800edd71a4500082b6377
😎 Deploy Preview https://deploy-preview-1268--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Hey Liam, as I mentioned on Slack, thanks so much for the PR and all the effort you put into it, really appreciated!

I updated the Cake script to run the tests sequentially for now (I know it's slower, but I have some thoughts on that below). Could I kindly ask you to merge the develop branch and revert the changes related to Cake and the pipeline for now? I'd prefer to keep the PRs separate.

I've been thinking about a few things:

  1. I think we should avoid pruning images after each test (assembly). I want to prevent removing all images on a developer’s machine when someone uses the Cake task.
    • I'd prefer using an MSBuild target (this works from a CLI and an IDE too) that developers can opt into, for example, using an environment variable. The MSBuild target could then parse log messages and prune only the images actually used by the tests.
  2. I changed the pipeline to run sequentially to make reading the log messages easier, it was quite messy and difficult to follow before. My idea is to spawn multiple runners for the different modules to run the tests in parallel (in the future). We already have the implementation to merge code coverage from multiple runners, and I think this would have a few advantages: it should run much faster, be more stable, and it would eliminate the need to free up disk space, so we don’t need to worry about the size of the images.

WDYT?

@TechLiam
Copy link
Contributor Author

TechLiam commented Oct 6, 2024

Hi @HofmeisterAn
This all makes sense I have merged back develop into my branch and removed any changes that are not papercut module related hopefully, everything else is good from your point of view. Sorry, this too a while, hectic week no time for open source committing :)

@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Oct 10, 2024
@HofmeisterAn HofmeisterAn changed the title Reintroducing Papercut container modual feat: Reintroduce Papercut module Oct 10, 2024
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thanks 🥳

@HofmeisterAn HofmeisterAn merged commit 159e4bf into testcontainers:develop Oct 10, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants