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

Add network option #1028

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

leizor
Copy link

@leizor leizor commented Feb 11, 2024

I was seeking a solution for writing tests that deal with multiple containers that all need to be able to talk to each other.

I found #534 but it looked like it might've gone stale so I decided to build upon the work that they had already started (I hope that's ok!).

Instead of creating the network while starting up containers, I added gnomock.StartNetwork to explicitly add a network. When creating containers, use gnomock.WithNetworkID to tell the container to attach to a created network. After the test is done, use gnomock.StopNetwork to remove the network.

@leizor
Copy link
Author

leizor commented Feb 11, 2024

I'm leaving a failing lint:

docker.go:317: Function 'createContainer' is too long (63 > 60) (funlen)
func (d *docker) createContainer(

It seems beyond the scope of this PR to do that refactor but let me know if you'd like me to take a crack at it!

@orlangure
Copy link
Owner

Hey @leizor and sorry for taking so long to respond.

First, thanks for working on it and for all the effort, I can see how this would be useful to people that use multi-container setup 😼

Regarding the suggested API, I do have a few concerns. I'm not a fan of adding code to explicitly create and remove networks, which is basically a proxy to docker client code. There is little added value. What I think would be really cool though is that you could pass network name without creating it, and gnomock would scan the list of networks, and create one if no networks with the requested name exist. So your tests won't have to deal with the IDs at all, you only specify which network name you need, and it is either used, or created for you. What do you think?

Of course then there would be an issue of dangling networks which aren't cleaned up. We would need to leverage the cleaner container for that, or simply document that the networks are created and never removed, at least for now.

Regarding the funlen linter warning, I think let's just drop it, it doesn't bring much value IMO.

Sorry again for taking so long. Please let me know if you would like to keep working on this, or is it too late 😿

@leizor
Copy link
Author

leizor commented Jul 17, 2024

@orlangure Sounds reasonable! I'll take a look at implementing those changes.

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