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 optional network setting #5

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

Conversation

jasonmead
Copy link

No description provided.

@jasonmead jasonmead requested a review from bpholt September 11, 2017 20:08
@bpholt
Copy link
Member

bpholt commented Sep 11, 2017

This PR addresses #4

@bpholt
Copy link
Member

bpholt commented Sep 11, 2017

I'd like to see some scripted-based integration tests demonstrating that the container created with the new network option is actually attached to that network.

It would be a good idea to add a negative test proving that it's not added unless it's asked for, but I see that we don't have a scripted test demonstrating the defaults for a container created by the plugin, so I'm ok with not adding the negative test for now. (If you want to add it, go for it.)

@bpholt
Copy link
Member

bpholt commented Sep 11, 2017

Is --network mutually exclusive with --link or can both exist for the same container? If they're mutually exclusive, I wonder if we should express that with the type system somehow?

Update: they are not mutually exclusive.

@bpholt
Copy link
Member

bpholt commented Sep 11, 2017

We should update the README with the new option before this is merged too.

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