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: Extend the "wait until file exists" API to distinguish between the test host and container filesystem #1009

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

maaex
Copy link
Contributor

@maaex maaex commented Oct 2, 2023

What does this PR do?

The existing implementation of the UntilFileExists WaitStrategy seems to look for a file in the host's file system instead of the container's.
This PR changes that so UntilFileExists looks for the file in the container's file system instead.

Why is it important?

Looking for a file on the host instead of the container seems like incorrect behavior.

@netlify
Copy link

netlify bot commented Oct 2, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 0d3f6cc
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/6542140a474d0e00088acc07
😎 Deploy Preview https://deploy-preview-1009--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.

@maaex maaex force-pushed the patch-1 branch 3 times, most recently from cd093ed to 37063ec Compare October 3, 2023 05:56
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.

Thank you for your pull request. This is some kind of on purpose, but I understand the confusion regarding the wait strategy. I would like to suggest the following in order to avoid breaking existing configurations:

  1. Enhance the UntilFileExists(string) documentation (interface) and make it more explicit that it checks for a file on the test host.
  2. Add an overloaded method that checks for the file inside the container.

We can also introduce two explicit APIs (wait strategies): One that checks on the test host and another that checks in the container, while deprecating the old one. My goal is to avoid introducing breaking changes without informing the developers.

@maaex
Copy link
Contributor Author

maaex commented Oct 4, 2023

Thank you for your pull request. This is some kind of on purpose, but I understand the confusion regarding the wait strategy. I would like to suggest the following in order to avoid breaking existing configurations:

  1. Enhance the UntilFileExists(string) documentation (interface) and make it more explicit that it checks for a file on the test host.
  2. Add an overloaded method that checks for the file inside the container.

We can also introduce two explicit APIs (wait strategies): One that checks on the test host and another that checks in the container, while deprecating the old one. My goal is to avoid introducing breaking changes without informing the developers.

Good point. Should I move the code into a UntilFileExistsInContainer(string) method (+ a related WaitStrategy class) maybe? That naming should make the distinction clearer between the two.

@HofmeisterAn
Copy link
Collaborator

Good point. Should I move the code into a UntilFileExistsInContainer(string) method (+ a related WaitStrategy class) maybe? That naming should make the distinction clearer between the two.

Yes, I think that would be a better and more explicit approach. I am currently thinking how we can enhance the wait strategy API in a broader sense, without the need for a complete overhaul. The wait strategy API has remained unchanged since its early days. Perhaps an approach similar to the HttpWaitStrategy, which allows more configurations, would be a more future-proof solution. We could consider implementing something like a FileSystemWaitStrategy, which accept a file path and a scope to determine whether it is a file within a container or a file on the test host.

@maaex
Copy link
Contributor Author

maaex commented Oct 20, 2023

I see that the HttpWaitStrategy has different usage pattern to the 'normal' wait strategies. Do we need to go there with this? Maybe something like Wait.ForUnixContainer().UntilFileExists("/foo/bar", FileSystem.Host) would suffice? And have the default scope value as Host to avoid breaking any existing functionality.

And do you have any suggestions for what to call the scope enum?

@HofmeisterAn
Copy link
Collaborator

Do we need to go there with this? Maybe something like

No, I think your suggestion is reasonable.

And do you have any suggestions for what to call the scope enum?

FileSystem.Host, FileSystem.Container sounds good 👍.

@maaex
Copy link
Contributor Author

maaex commented Oct 31, 2023

I have updated the code as per our discussion. I was unsure of where to put the FileSystem enum but ended up putting it under TestContainers.Configurations as I felt it was a more generic thing than WaitStrategies. What do you think?

@HofmeisterAn
Copy link
Collaborator

I took a brief look. It looks very good. Great first contribution, thanks. I won't be able to finalize the review today, but I can certainly do it tomorrow.

@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Nov 1, 2023
@HofmeisterAn HofmeisterAn changed the title UntilFilesExists should look for file in container feat: Extend the "wait until file exists" API to distinguish between the test host and container filesystem Nov 1, 2023
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 for the PR and addressing the requests. Great first contribution, thanks 👍.

@HofmeisterAn HofmeisterAn merged commit 871a9cd into testcontainers:develop Nov 1, 2023
9 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.

None yet

3 participants