Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Detect iSCSI devices #300

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Detect iSCSI devices #300

merged 1 commit into from
Mar 15, 2022

Conversation

avishayt
Copy link
Contributor

Fixes: #297

@avishayt avishayt closed this Feb 17, 2022
@avishayt avishayt reopened this Feb 22, 2022
ffromani
ffromani reviewed Mar 11, 2022
View reviewed changes
Copy link
Collaborator

@ffromani ffromani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR! indeed acknowledging ISCSI devices would be nice.
It would be real cool to add unit test(s) to exercise this code, even if it's a simple change, to make it unlikely to break in the future

pkg/block/block_linux.go Show resolved Hide resolved
@avishayt
Copy link
Contributor Author

thanks for the PR! indeed acknowledging ISCSI devices would be nice. It would be real cool to add unit test(s) to exercise this code, even if it's a simple change, to make it unlikely to break in the future

Added a unit test - thanks!

@ffromani
Copy link
Collaborator

thanks for the updates! codewise LGTM - we can further improve the testing in the future if needed/feasible.
The only thing left is the DCO check which is failing - could you PTAL to the instructions (https://github.com/jaypipes/ghw/pull/300/checks?check_run_id=5556037529) and see if there is a easy fix? feel free to get in touch if instructions are not clear

@avishayt avishayt force-pushed the iscsi branch 2 times, most recently from b245dcd to 477941d Compare March 15, 2022 15:35
@avishayt
Detect iSCSI devices
3171315 
Fixes: jaypipes#297
Signed-off-by: Avishay Traeger <[email protected]>
@avishayt
Copy link
Contributor Author

thanks for the updates! codewise LGTM - we can further improve the testing in the future if needed/feasible. The only thing left is the DCO check which is failing - could you PTAL to the instructions (https://github.com/jaypipes/ghw/pull/300/checks?check_run_id=5556037529) and see if there is a easy fix? feel free to get in touch if instructions are not clear

Fixed, thanks for the pointer

ffromani
ffromani approved these changes Mar 15, 2022
View reviewed changes
Copy link
Collaborator

@ffromani ffromani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your contribution and your time!

@ffromani ffromani merged commit 50f78dc into jaypipes:main Mar 15, 2022
jaypipes
jaypipes reviewed Mar 24, 2022
View reviewed changes
Copy link
Owner

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned on #300, SCSI is not a drive type (iSCSI or otherwise...). It is a storage controller interface. The drive type is hard-disk drive (HDD) and the storage controller should be SCSI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaypipes jaypipes jaypipes left review comments

@ffromani ffromani ffromani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Properly detect iSCSI disks
3 participants
@avishayt @ffromani @jaypipes