-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: CheckVolumeExists() optimization and new GetVolumeWwn() #18
Conversation
jskazinski
commented
May 25, 2022
- Optimization to CheckVolumeExists() to show one volume, which produces an error log entry, but that is used to indicate that the volume does not exist
- Added GetVolumeWwn() to execute a storage controller command to retrieve the WWN. This results in a slight addition of execution time, but provides the ability to locate operating system devices based on WWN rather than IP Address and LUN.
Signed-off-by: Joe Skazinski <[email protected]>
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.
looks good
api.go
Outdated
func (client *Client) GetVolumeWwn(volumeName string) (string, error) { | ||
|
||
wwn := "" | ||
response, _, err := client.ShowVolumes(volumeName) |
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.
just a nit, but i think you can use the 2nd result from ShowVolumes to simplify the error checking that follows?
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.
Good point, I will adjust to use the return status provided.
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.
Looks good
Signed-off-by: Joe Skazinski <[email protected]>
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.
Looks good, LGTM!