api: name attachment field according to surrounding style#2385
Merged
dperny merged 1 commit intomoby:masterfrom Sep 26, 2017
Merged
api: name attachment field according to surrounding style#2385dperny merged 1 commit intomoby:masterfrom
dperny merged 1 commit intomoby:masterfrom
Conversation
Contributor
Author
|
Addresses some missed areas from #2363. |
stevvooe
force-pushed
the
remove-unnecessary-field-naming
branch
from
f4707aa to
65d486d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2385 +/- ##
==========================================
- Coverage 66.04% 60.53% -5.51%
==========================================
Files 80 128 +48
Lines 14583 26260 +11677
==========================================
+ Hits 9631 15897 +6266
- Misses 4163 8959 +4796
- Partials 789 1404 +615 |
ijc
reviewed
Sep 22, 2017
| @@ -87,8 +87,8 @@ type NetworkAllocator interface { | |||
| // DeallocateLBAttachment Deallocates a load balancer endpoint for the node | |||
| DeallocateLBAttachment(node *api.Node, networkAttachment *api.NetworkAttachment) error | |||
Contributor
There was a problem hiding this comment.
Is leaving this and the AllocateLB just above deliberate?
Contributor
Author
There was a problem hiding this comment.
Nope. I'll hunt these down.
Contributor
|
Sorry for missing this feedback in the original review. |
Contributor
Author
|
@pradipd No worries. I blame github's review feature. |
stevvooe
force-pushed
the
remove-unnecessary-field-naming
branch
from
65d486d to
99c00a1
Compare
aaronlehmann
approved these changes
Sep 23, 2017
|
|
||
| //IsLBAttachmentAllocated If lb endpoint is allocated on the node | ||
| IsLBAttachmentAllocated(node *api.Node, networkAttachment *api.NetworkAttachment) bool | ||
| //IsAttachmentAllocated If lb endpoint is allocated on the node |
Collaborator
There was a problem hiding this comment.
Add a space between // and the comment?
Rather than invent style conventions, we should follow what has been done before. This removes the unnecessary "lb" from the `attachments` field. Network attachements are a general concept, not for specific use cases. If we need to identify different attachments, we should tag the attachement itself, rather than label fields. This will ensure that we can grow without introducing `xxx_attachements` fields for each new use case. This was requested in review but seems to have been dropped. Signed-off-by: Stephen J Day <[email protected]>
stevvooe
force-pushed
the
remove-unnecessary-field-naming
branch
from
99c00a1 to
a3eac73
Compare
thaJeztah
approved these changes
Sep 26, 2017
Member
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM (not a maintainer here)
Collaborator
|
LGTM |
pradipd
added a commit
to pradipd/moby
that referenced
this pull request
Sep 26, 2017
…t#2385 Signed-off-by: Pradip Dhara <[email protected]>
andrewhsu
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Sep 28, 2017
…t#2385 Signed-off-by: Pradip Dhara <[email protected]> Upstream-commit: d00a07b Component: engine
salah-khan
pushed a commit
to salah-khan/moby
that referenced
this pull request
Nov 15, 2017
…t#2385 Signed-off-by: Pradip Dhara <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rather than invent style conventions, we should follow what has been
done before. This removes the unnecessary "lb" from the
attachmentsfield. Network attachements are a general concept, not for specific use
cases. If we need to identify different attachments, we should tag the
attachement itself, rather than label fields. This will ensure that we
can grow without introducing
xxx_attachementsfields for each new usecase.
This was requested in review but seems to have been dropped.
Signed-off-by: Stephen J Day [email protected]