-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Document CLI support for per interface sysctls #4994
Document CLI support for per interface sysctls #4994
Conversation
8248665
to
e91f6e2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4994 +/- ##
==========================================
+ Coverage 61.09% 61.33% +0.24%
==========================================
Files 298 295 -3
Lines 20675 20701 +26
==========================================
+ Hits 12631 12697 +66
+ Misses 7147 7101 -46
- Partials 897 903 +6 |
e91f6e2
to
5474acf
Compare
5474acf
to
774ed6a
Compare
The test didn't do anything useful... - Despite its name it used newCreateCommand() instead of newConnectCommand() with create flags/options instead of connect. - There was no fake networkCreateFunc(), so the result of the 'connect' wasn't checked. - The fake networkConnectFunc() was never called, so didn't spot the problem. Signed-off-by: Rob Murray <rob.murray@docker.com>
Support for connecting more than one network using the container run command was added in v25.0 for API > 1.44 - describe that in the docs. Signed-off-by: Rob Murray <rob.murray@docker.com>
f4cef3e
to
9f7d811
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@thaJeztah does this LGTY? |
Signed-off-by: Rob Murray <rob.murray@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
9f7d811
to
d5d94e4
Compare
Updated to reflect a last-minute change of heart on the sysctl naming ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- What I did
com.docker.network.endpoint.sysctls
, to support per-interface sysctls.The extended
--network
syntax is needed, but wasn't documented.The quoting needed to set more than one sysctl using
docker [create|run] --network driver-opt
ordocker network connect --driver-opt
isn't obvious - so, added tests and examples to the docs.- How I did it
TestNetworkConnectWithFlags
- because it didn't do anything, and is needed to test the new option.--network
syntax.- How to verify it
New tests added.
- Description for the changelog