Conversation
|
@avishayt I apologize for the delay in getting to these reviews, sir. Is there a reason you closed out your PRs? |
|
Wasn't sure the project was still active :) |
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
thanks for the change! the code per se looks good! It would be real nice to have some tests to cover this change, in order to avoid future breakage.
It would be also real nice to have an example of the sysfs directory tree involved here.
Perhaps ghw-snapshot can help to capture that?
Fixes: jaypipes#298 Signed-off-by: Avishay Traeger <avishay@redhat.com>
|
Added unit tests |
| SerialNumber string `json:"serial_number"` | ||
| WWN string `json:"wwn"` | ||
| Partitions []*Partition `json:"partitions"` | ||
| Members []string `json:"members"` |
There was a problem hiding this comment.
LGTM but let's hear from @jaypipes because of the API-level change
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
LGTM, thanks for the updates.
| // TODO(jaypipes): Move this into diskTypes() once abstracting | ||
| // diskIsRotational for ease of unit testing | ||
| if !diskIsRotational(ctx, paths, dname) { | ||
| if driveType != DRIVE_TYPE_MAPPER && !diskIsRotational(ctx, paths, dname) { |
There was a problem hiding this comment.
we will probably need to review this flow more deeply, but that's totally material for another PR. This change LGTM for this PR
3 participants
Fixes: #298