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: allow setting custom IP address for container #587

Closed
wants to merge 1 commit into from

Conversation

oktalz
Copy link
Contributor

@oktalz oktalz commented Oct 25, 2022

this PR adds option to specify custom IP for a container

some prerequisites are needed, but those are regular as with doing the same with docker or docker compose
this is possible to use only when using user defined subnets, but error message is quite clear if anyone tries to use it without.

reason why i need this feature is quite simple:
I have some complex tests where I need to know in advance IP addressees of some containers before starting anything.

However I can only do that currently by starting the containers, getting the IPs of all containers, stopping it, clearing data, setting all IPs in configs of multiple containers and then start containers again and proceed with test

so the test itself can be much simpler if I can define all addresses in advance, create configs, build containers with already known IPs and just focus on actual testing of a feature.
Reason why IP needs to be fixed (and not hostname or some discovery ) is due to protocols used and for example security (and I do something similar not only in tests)
Also its much easier to set the "wrong" IP and test implementation where security is good enough to not allow that.

I also added test where i test both creating container in custom subnet + creating container with custom IP (for test is just next available IP) in same network.

@oktalz oktalz requested a review from a team as a code owner October 25, 2022 21:26
@oktalz oktalz force-pushed the feat/containerip branch 2 times, most recently from eda8f0e to 56fdf7c Compare October 26, 2022 18:31
@mdelapenya
Copy link
Member

HI @oktalz thanks for your contribution. I'd like to know more about your use case to understand the rationale behind this addition.

Could you please update the description of the PR with the motivation? 🙏

@oktalz
Copy link
Contributor Author

oktalz commented Oct 26, 2022

@mdelapenya description was updated

@mdelapenya
Copy link
Member

Hi @oktalz I'd like to thank you for your time submitting this PR.

Unfortunately, I don't find commonalities in the other language implementations of Testcontainers (Java and .NET as the most important references) to consider this a canonical use case that should be added to the library.

Having a fixed IP seems a bit specific for your use case, so I'm not sure if adding this capability to the library could lead to misconfigurations. Nevertheless, we are more than open to creating a Discussion, so that the community share their thoughts about this.

Please let me know what you think, so we can move 1) forward on the discussion or 2) close the issue.

Thanks!

@oktalz
Copy link
Contributor Author

oktalz commented Oct 27, 2022

@mdelapenya one could wonder why is this any different than allowing the completely the same thing on networks (also setting IPAM there) since both features are available to configure via docker API

the way how it is implemented is that it will not impact you in any way if you do not use it, underline struct already accept this params, its just a matter off passing that to it

I can open a discussion and would love to see it.
i already use my forked repo and will continue to do so, but would love to see that upstream.

also, I'm not really to eager to implement this in all other languages in order to this be accepted in Go.
But I will definitely open a discussion about it.

@oktalz
Copy link
Contributor Author

oktalz commented Oct 27, 2022

created Discussion about it
#590

this is possible only when using user defined subnets
@oktalz
Copy link
Contributor Author

oktalz commented Nov 21, 2022

different approach was chosen discussions/628 so I'll close this PR

@oktalz oktalz closed this Nov 21, 2022
@oktalz oktalz deleted the feat/containerip branch November 22, 2022 20:55
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