Clarify that NetworkAttachment target is an ID #1602
Conversation
PR moby#1600 added a FindNetwork function to the store package to resolve a NetworkAttachment's target by ID, name, or name prefix. I think this is based on a misleading comment in the NetworkAttachment definition saying that the target can be a name or an ID. In fact, it's always an ID, and existing code passes it directly to GetNetwork. Clarify the comment and remove FindNetwork. Signed-off-by: Aaron Lehmann <[email protected]>
Current coverage is 54.13% (diff: 100%)@@ master #1602 diff @@
==========================================
Files 84 84
Lines 13952 13944 -8
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 7497 7548 +51
+ Misses 5443 5401 -42
+ Partials 1012 995 -17
|
|
@aaronlehmann When testing the fix #1600 I recalled that if I specify Let me test again. If it is not needed I am happy this gets removed. |
|
@aaronlehmann looks good to me |
|
LGTM |
| // Target specifies the target network for attachment. This value may be a | ||
| // network name or identifier. Only identifiers are supported at this time. | ||
| // Target specifies the target network for attachment. This value must be a | ||
| // network ID. |
There was a problem hiding this comment.
I don't think this is correct. The reason why this was called Target is for a possible move over in the future to use names or both name or ID. So I believe the previous comment should still be retained. But I am fine with removing FindNetwork since we don't expect a name with the current implementation.
PR #1600 added a FindNetwork function to the store package to resolve a
NetworkAttachment's target by ID, name, or name prefix. I think this is
based on a misleading comment in the NetworkAttachment definition saying
that the target can be a name or an ID. In fact, it's always an ID, and
existing code passes it directly to GetNetwork.
Clarify the comment and remove FindNetwork.
cc @aboch @mrjana @aluzzardi