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: Use exposed ports specified in image if it is not specified in ContainerRequest #468

Merged
merged 8 commits into from
Jul 13, 2022

Conversation

fiftin
Copy link
Contributor

@fiftin fiftin commented Jun 17, 2022

Relates to #343

Example:

# Dockerfile
EXPOSE 11211
CMD ["memcached"]

Old:

port := "11211/tcp"
rq := testcontainers.ContainerRequest{
	Image:        "memcached:1.6.10",
	ExposedPorts: []string{port},
	WaitingFor:   wait.ForListeningPort(nat.Port(port)),
}

New:

rq := testcontainers.ContainerRequest{
	Image:        "memcached:1.6.10",
	WaitingFor:   wait.ForListeningPort(""),
}

@fiftin
Copy link
Contributor Author

fiftin commented Jul 11, 2022

I will be very grateful if you check my PR ASAP. I can not continue to work on the PR after Jul 13. Thank you!

Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

I added two small nits. The PR looks good to me. My only concern is about the call to the wait.ForListeningPort("") method with an empty string. Not sure if adding a new method, something like wait.ForExposedPort() would me it more usable though, but I'd like to hear your thoughts on it.

It's not a blocker for this PR, which I will approve once we discuss about it and find ourselves in a closed path.

wait/host_port.go Outdated Show resolved Hide resolved
wait/log_test.go Show resolved Hide resolved
fiftin and others added 2 commits July 12, 2022 13:13
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@fiftin
Copy link
Contributor Author

fiftin commented Jul 12, 2022

@mdelapenya ,

We have HostPortStrategy struct which has Port field. Method wait.ForListeningPort just instantiate this struct with provided port.

Yes, we can add method wait.ForExposedPort() but it would an alias for wait.ForListeningPort("").

@mdelapenya
Copy link
Collaborator

Yes, we can add method wait.ForExposedPort() but it would an alias for wait.ForListeningPort("").

I want it to be simple to use. I we have to do the mental effort of passing an empty string to wait for a port declared at the Dockerfile, then I think it would make it difficult to remember/easy to forget about it, unless the docs/tests are crystal clear. I think we are missing it in the docs, as you already added the example in the tests 👍

If you find it having the alias confusing, then it's OK to keep the method with the empty string, but well documented.

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #468 (37e1e95) into main (28706cb) will increase coverage by 4.10%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
+ Coverage   65.55%   69.65%   +4.10%     
==========================================
  Files          19       21       +2     
  Lines        1199     2040     +841     
==========================================
+ Hits          786     1421     +635     
- Misses        305      495     +190     
- Partials      108      124      +16     
Impacted Files Coverage Δ
wait/wait.go 100.00% <ø> (ø)
wait/host_port.go 48.42% <15.00%> (-7.72%) ⬇️
docker.go 71.09% <90.00%> (+5.39%) ⬆️
container.go 80.72% <0.00%> (-6.52%) ⬇️
compose.go 74.04% <0.00%> (-2.73%) ⬇️
logger.go 100.00% <0.00%> (ø)
mounts.go 100.00% <0.00%> (ø)
testing.go 0.00% <0.00%> (ø)
wait/multi.go 0.00% <0.00%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28706cb...37e1e95. Read the comment docs.

@fiftin
Copy link
Contributor Author

fiftin commented Jul 12, 2022

Added wait.ForExposedPort.

Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

Last minute changes in docs, but other than that LGTM! Thanks @fiftin for your hard work here

wait/host_port.go Show resolved Hide resolved
wait/host_port.go Outdated Show resolved Hide resolved
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Jul 13, 2022
@mdelapenya mdelapenya self-assigned this Jul 13, 2022
@mdelapenya mdelapenya merged commit 0347c2a into testcontainers:main Jul 13, 2022
@mdelapenya
Copy link
Collaborator

Thanks for your patience @fiftin!! It's merged now 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants