This repository was archived by the owner on Aug 15, 2022. It is now read-only.
Machine mount - add mount list handler to machine group - #10382
Merged
Conversation
ppknap
force-pushed
the
machine_mount_machinegroup_list
branch
from
January 24, 2017 16:46
5a978d4 to
dc8e979
Compare
rjeczalik
reviewed
Jan 25, 2017
| type ListMountRequest struct { | ||
| // ID is an optional identifier for the remote machine. If set, only | ||
| // mounts related to this machine will be returned. | ||
| ID machine.ID `json:"id"` |
rjeczalik
reviewed
Jan 25, 2017
| ID machine.ID `json:"id"` | ||
|
|
||
| // MountID is an optional identifier of a mount which is meant to be listed. | ||
| MountID mount.ID `json:"mountID"` |
rjeczalik
reviewed
Jan 25, 2017
| if req.ID != "" { | ||
| mounts, err := g.mount.All(req.ID) | ||
| if err != nil { | ||
| return res, nil |
Contributor
There was a problem hiding this comment.
Why the early return here? Shouldn't we try req.MountID as well? If req.ID and req.MountID are exclusive, then some check is missing.
Contributor
There was a problem hiding this comment.
If an empty mount from L205 is eliminated then indeed we could early return here.
rjeczalik
approved these changes
Jan 25, 2017
rjeczalik
reviewed
Jan 25, 2017
|
|
||
| mms = map[mount.ID]mountsMachine{ | ||
| req.MountID: mountsMachine{m: m, id: id}, | ||
| } |
Contributor
There was a problem hiding this comment.
If g.mount.All failed we'd be adding an empty mount here.
ppknap
force-pushed
the
machine_mount_machinegroup_list
branch
from
January 25, 2017 06:37
dc8e979 to
cee851e
Compare
cihangir
reviewed
Jan 25, 2017
| // ListMount checks the status of mounts and returns their infos. This function | ||
| // guarantees that if returned error is nil, response Mounts field is always | ||
| // non-nil. It doesn't fail if provided filters contain incorrect values. | ||
| func (g *Group) ListMount(req *ListMountRequest) (*ListMountResponse, error) { |
Contributor
There was a problem hiding this comment.
could you separate this function into smaller logical parts?, it is hard ( for me ) to follow it
cihangir
reviewed
Jan 25, 2017
|
|
||
| for name, test := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| listMountRes, err := g.ListMount(&test.LMReq) |
Contributor
There was a problem hiding this comment.
could you add t.Parallel here?
added 6 commits
January 26, 2017 02:13
ppknap
force-pushed
the
machine_mount_machinegroup_list
branch
from
January 26, 2017 01:37
cf7633e to
0c1c248
Compare
Contributor
Author
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 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.
Depends on:
#10377Code coverage: 51.3% of statements
How Has This Been Tested?
Unit tests.
Screenshots (if appropriate):
none
Types of changes