[node-patches] Change in ovirt-node[master]: find_grub_cfg(): Add elif statement to get correct cfg_path ...

[fabiand at redhat.com]

Fabian Deutsch has posted comments on this change.

Change subject: find_grub_cfg(): Add elif statement to get correct cfg_path for efi_boot
......................................................................


Patch Set 9: Code-Review-1

(2 comments)

http://gerrit.ovirt.org/#/c/26418/9/src/ovirt/node/utils/system.py
File src/ovirt/node/utils/system.py:

Line 935:         for dirName, subdirList, fileList in os.walk(cfg_path):
Line 936:             for f in fileList:
Line 937:                 if f.endswith(".conf") or f.endswith(".cfg"):
Line 938:                     cfg = "%s/%s" % (dirName, f)
Line 939: 
Please add some logging here so we see in the logs what filenames are tried.
Line 940:         if not cfg:
Line 941:             raise RuntimeError("Failed to find the path for grub.[cfg|conf]")
Line 942:         return File(cfg)
Line 943: 


Line 936:             for f in fileList:
Line 937:                 if f.endswith(".conf") or f.endswith(".cfg"):
Line 938:                     cfg = "%s/%s" % (dirName, f)
Line 939: 
Line 940:         if not cfg:
Why did you drop the check if the file exuists?
IMO it makes sense to check that case?
Line 941:             raise RuntimeError("Failed to find the path for grub.[cfg|conf]")
Line 942:         return File(cfg)
Line 943: 
Line 944:     class Arguments(base.Base):


-- 
To view, visit http://gerrit.ovirt.org/26418
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I299fb4906d8d2e947130daea1de570a9347bb202
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: hadong <hadong0720 at gmail.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at redhat.com>
Gerrit-Reviewer: Joey Boggs <jboggs at redhat.com>
Gerrit-Reviewer: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: hadong <hadong0720 at gmail.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes