-
-
Notifications
You must be signed in to change notification settings - Fork 290
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: Add http wait strategy #552
feat: Add http wait strategy #552
Conversation
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.
Looks pretty good, thanks! Just a few minor comments.
using DotNet.Testcontainers.Builders; | ||
using JetBrains.Annotations; | ||
|
||
public static class ExternalWaitStrategyExtension |
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.
Any reason why you choose an extension? I think we can put this into WaitForContainerOS
.
var response = await httpClient.SendAsync( | ||
new HttpRequestMessage( | ||
this.request.Method, | ||
this.BuildRequestUri(testcontainers.Hostname, testcontainers.GetMappedPublicPort(this.request.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.
I think you can use the UriBuilder here. If you create HttpRequestMessage
in the HttpWaitRequest
you probably don't need it here at all.
var response = await httpClient.SendAsync( | ||
new HttpRequestMessage( | ||
this.request.Method, | ||
this.BuildRequestUri(testcontainers.Hostname, testcontainers.GetMappedPublicPort(this.request.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.
Maybe you can pass the Hostname
to something like HttpRequestMessage Build(string hostname, int mappedPublicPort)
. Or WithHostname(...)
. That the builder creates the HttpRequestMessage
. Then you probably don't need the nested Builder
class too.
{ | ||
private const string DefaultPath = "/"; | ||
|
||
private readonly int 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.
I think you can use the UriBuilder here and store the values in the builder fields.
|
||
public HttpMethod Method { get; } | ||
|
||
public ISet<int> StatusCodes { get; } |
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.
ISet<HttpStatusCode>
?
I will close this in favor of #717. I've added you as a co-author. Thanks for your contribution. |
Adds external http wait strategy as an extension of
IWaitForContainerOS
. In doing so, http wait strategy can be chained with OS specific ones. I am opening this PR for feedback.Todo
Example