NVMe Command constant needs to use full path #53
Conversation
karthikk92
force-pushed
the
nvme_update
branch
from
143c850 to
12068f9
Compare
karthikk92
requested review from
boyamurthy,
bpjain2004,
coulof,
delldubey,
francis-nijay,
harishp8889,
hoppea2,
kumarp20,
nidtara,
prablr79,
tdawe,
santhoshhs10,
shanmydell,
tssushma,
alikdell,
atye,
shaynafinocchiaro and
xuluna
as code owners
gonvme_tcp_fc.go
Outdated
| if nvme.getChrootDirectory() != "/" { | ||
| command = append([]string{"chroot", nvme.getChrootDirectory()}, "which", DefaultNVMeCommand) | ||
| } else { | ||
| command = []string{"which", DefaultNVMeCommand} | ||
| } | ||
| log.Infof("NVMe %s",command) | ||
| path, err := exec.Command(command[0], command[1:]...).Output() |
There was a problem hiding this comment.
This does not solve the original problem reported in the GitHub issue. You are still at the mercy of finding the command from the PATH. My interpretation of the original issue is to hard code the path to the nvme command. You could instead try some well known paths, e.g. [/bin/nvme, /usr/bin/nvme, ...] and pick the first one.
There was a problem hiding this comment.
@donatwork as discussed yesterday, we tried checking the different possible paths as its updated in commits, but its not working as expected as it checks the path locally and throws path does not exist.
There was a problem hiding this comment.
@donatwork as discussed code changes done with list of paths now and updated in the PR.
karthikk92
force-pushed
the
nvme_update
branch
2 times, most recently
from
a2ded98 to
95be92c
Compare
karthikk92
force-pushed
the
nvme_update
branch
from
9ac0de6 to
37b96bf
Compare
|
Will add Single PR for Both defect fix and UT coverage |
Description
NVMeCommand constant needs to use full path
GitHub Issues
List the GitHub issues impacted by this PR:
Checklist:
How Has This Been Tested?
Image build pointing to gonvme branch and installed powerstore driver and ran basic scale testing
UTs added and now coverage increased from 20% to 21.3%
UTS covered under this PR: #54, once this is merged, will merge present PR.