Changing networkAttachment to address PR comment#2183
Changing networkAttachment to address PR comment#2183aaronlehmann merged 1 commit intomoby:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2183 +/- ##
==========================================
+ Coverage 60.12% 60.15% +0.02%
==========================================
Files 119 119
Lines 19794 19785 -9
==========================================
Hits 11902 11902
+ Misses 6545 6534 -11
- Partials 1347 1349 +2 |
api/objects.proto
Outdated
| @@ -257,7 +257,7 @@ message NetworkAttachment { | |||
| repeated string aliases = 3; | |||
|
|
|||
| // Map of all the driver options for this network | |||
There was a problem hiding this comment.
Please update this comment.
Keeping the field comments correct and up to date is important. It helps us to document these fields and make sure they are operating correctly.
Addressing comment from stevvooe on moby#2176 Signed-off-by: Abhinandan Prativadi <[email protected]>
|
@stevvooe I have addressed the comment. PTAL |
| // Map of all the driver options for this network | ||
| map<string,string> driver_opts = 4; | ||
| // Map of all the driver attachment options for this network | ||
| map<string,string> driver_attachment_opts = 4; |
There was a problem hiding this comment.
This comment really needs to cover the questions I keep asking about this feature:
// DriverAttachmentOpts are passed to the network driver upon attaching to the network.
// These are specific to the driver and interpreted according to the semantics of that driver.
//
// Some examples of this include...
//
// Avoid using this field when...
|
LGTM We can merge this one, but please submit a PR with a comment that isn't the bare minimum. It should explain the ownership and lifecycle of the field, including any caveats or design changes that might occur in the future. This allows us to understand the behavior of the field without having to chase it down all over the code base. |
|
@abhinandanpb can you pls udpate the comment so that we can get this one asap to unblock the moby PRs for the 17.06 code-freeze ? |
|
@mavenugo We can go ahead and merge without the comment fix. |
4 participants
Addressing comment from stevvooe on #2176
Signed-off-by: Abhinandan Prativadi [email protected]
ping @stevvooe , @mavenugo