Skip to content

Conversation

@justinwoo
Copy link
Contributor

@justinwoo justinwoo commented Nov 18, 2025

mostly for testing

@justinwoo justinwoo requested a review from alexbiehl as a code owner November 18, 2025 11:39
Copy link
Collaborator

@LaurentRDC LaurentRDC 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 your contribution!

-- This will fail if port 5432 or 5433 is already bound on the host.
--
-- @since 0.5.1.0
setPortBindings :: [(Int, Port)] -> ContainerRequest -> ContainerRequest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason to have NO port bindings, i.e. the empty list? Otherwise, I'd like to see a NonEmpty (Int, Port) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mostly just to not have to deal with wrapping no op in the user level

redisContainer <-
run $
containerRequest redis
& setPortBindings [(16379, "6379/tcp")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's interesting that you can specify a port and protocol. Can we add this to the example in the setPortBindings docstring?

in throw $
containerPort Container {id, inspectOutput, containerPortBindings} requestedPort@(Port {port, protocol}) =
-- First check if there's a fixed binding for this port
case find (\(_, boundPort) -> boundPort == requestedPort) containerPortBindings of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: i want to be able to handle this at the user level instead

--
-- This will fail if port 5432 or 5433 is already bound on the host.
--
-- @since 0.5.1.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO doc string is probably wrong

@justinwoo justinwoo marked this pull request as draft November 18, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants