-
Notifications
You must be signed in to change notification settings - Fork 615
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
api: name attachment field according to surrounding style #2385
Conversation
Addresses some missed areas from #2363. |
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 |
@@ -87,8 +87,8 @@ type NetworkAllocator interface { | |||
// DeallocateLBAttachment Deallocates a load balancer endpoint for the node | |||
DeallocateLBAttachment(node *api.Node, networkAttachment *api.NetworkAttachment) error |
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.
Is leaving this and the AllocateLB
just above deliberate?
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.
Nope. I'll hunt these down.
Sorry for missing this feedback in the original review. |
@pradipd No worries. I blame github's review feature. |
65d486d
to
99c00a1
Compare
|
||
//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 |
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.
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 <stephen.day@docker.com>
99c00a1
to
a3eac73
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 (not a maintainer here)
LGTM |
…t#2385 Signed-off-by: Pradip Dhara <pradipd@microsoft.com>
…t#2385 Signed-off-by: Pradip Dhara <pradipd@microsoft.com>
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 usecase.
This was requested in review but seems to have been dropped.
Signed-off-by: Stephen J Day stephen.day@docker.com