Skip to content

Fix test using com.docker.network.mtu#1907

Merged
fcrisciani merged 1 commit intomoby:masterfrom
thaJeztah:fix-faulty-test
Sep 13, 2017
Merged

Fix test using com.docker.network.mtu#1907
fcrisciani merged 1 commit intomoby:masterfrom
thaJeztah:fix-faulty-test

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

This should be com.docker.network.driver.mtu

fixes #1906

ping @abhinandanpb PTAL

This should be `com.docker.network.driver.mtu`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request Aug 14, 2017
@abhi
Copy link
Copy Markdown
Contributor

abhi commented Aug 14, 2017

LGTM

@fcrisciani
Copy link
Copy Markdown

@thaJeztah should we investigate why the test is passing?

@thaJeztah
Copy link
Copy Markdown
Member Author

Yes, if you have thoughts, definitely 👍

@abhi
Copy link
Copy Markdown
Contributor

abhi commented Aug 14, 2017

since its an option I dont there is a check to verify.

@fcrisciani
Copy link
Copy Markdown

don't have much context in this area, but I guess if the option is for a driver would have to contain the .driver. string I imagine, in that case I would say that we do not validate at the level of libnetwork, but if that string is not there, will probably mean that is a libnetwork option, so we can actually validate it and return an error if not recognized. What do you guys think? @thaJeztah @abhinandanpb

@abhi
Copy link
Copy Markdown
Contributor

abhi commented Aug 15, 2017

I think for this test if we want to check if mtu is set appropriately which would also mean we would have validated the option - we can add a check network.mtu == 1600 and network is returned by controller.NewNetwork

@fcrisciani
Copy link
Copy Markdown

I was more referring as a general approach more than the test itself, how to avoid to accept options that won't create any effect but will simply be silently ignored. As a first line of defense check that the option at least exists and if not return an error.

@vieux
Copy link
Copy Markdown
Contributor

vieux commented Aug 16, 2017

LGTM

@thaJeztah
Copy link
Copy Markdown
Member Author

@fcrisciani anything you want me to change in this PR, or good to go?

@fcrisciani fcrisciani merged commit 6d09846 into moby:master Sep 13, 2017
@thaJeztah thaJeztah deleted the fix-faulty-test branch September 14, 2017 00:03
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.

Faulty test?

4 participants