feat(wait): skip internal host port check#2691
feat(wait): skip internal host port check#2691mdelapenya merged 2 commits intotestcontainers:mainfrom stevenh:feat/skip-internal-port-check
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
wait/host_port.go
Outdated
|
|
||
| // ForExposedPortOnly returns a host port strategy that waits for the given port | ||
| // to be exposed only, it does not wait for the port to be bound inside the container. | ||
| func ForExposedPortOnly(port nat.Port) *HostPortStrategy { |
There was a problem hiding this comment.
I could see the value in adding a new constructor function, but I'd prefer consistency across the different wait strategies using the fluid builder pattern, something like this:
wait.ForListeningPort().SkipInternalCheck() // or any other relevant method nameI think this would be more consistent. Thoughts?
There was a problem hiding this comment.
Personally I find that needlessly verbose for a common option, so opted to follow the existing example set by ForListeningPort and ForExposedPort which admittedly only configure the port.
It could also be a bit confusing because the use case for this is when the port isn't actually listening.
What do you recon?
There was a problem hiding this comment.
We can probably find a better name:
- ForExternalPort, using the comments you added for the current
Onlyfunction - ForNotBoundPort
- ForPort
- ...
There was a problem hiding this comment.
Agreed the Only does seem unnecessary, how about just ForExposedPort as that matches the existing docker terminology so makes it clear what its looking to achieve?
There was a problem hiding this comment.
Mmm we already have the ForExposedPort that's why I'd add the fluid builder option to the strategy. In v1, we could go with functional options instead of the builder pattern for wait strategies.
There was a problem hiding this comment.
Yep forgot I added Only for that reason and naming things is hard :)
Think I'm gonna do a 180, should we do the builder SkipInternalCheck and then review the approach for v1?
There was a problem hiding this comment.
Went for WithoutInternalCheck as the SkipInternalCheck conflicts with the field name.
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM! will merge once the comment on private Vs public fields is addressed.
Thanks agains for your hard work
Add the ability to skip the internal host port check when performing as HostPortStrategy check. This is useful when a container does bind to the internal port until additional conditions are met. Also improve wait for port function documentation while there.
|
Given we changed the field to be private, we can declare the new function as Thanks |
|
@stevenh the CI passed, so if my changes are good to you, then we can merge, thanks! |
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
Looks good to me!
mdelapenya
added
the
feature
* main: fix: config via environment (#2725) fix(redpanda): race condition on port check (#2692) fix: logging restart (#2697) fix!: docker authentication setup (#2727) chore: improve error wrapping (#2720) chore: run make tests in verbose mode (#2734) chore(deps): bump github.com/docker/docker from 27.1.0+incompatible to 27.1.1+incompatible (#2733) fix(kafka): port race on start (#2696) docs: fix broken doc tags (#2732) fix: nginx request failures (#2723) fix(compose): container locking (#2722) fix(wait): log test timeout (#2716) chore: increase timeout values (#2719) chore: remove unused parameters (#2721) chore(mockserver): silence warning about internal port (#2730) feat(wait): skip internal host port check (#2691)
2 participants
Add the ability to skip the internal host port check when performing as HostPortStrategy check. This is useful when a container does bind to the internal port until additional conditions are met.