[node-patches] Change in ovirt-node[master]: edit-node: handle the "file://" type of baseurl for yum repo

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: edit-node: handle the "file://" type of baseurl for yum repo
......................................................................


Patch Set 9: Code-Review-1

(5 comments)

A couple of notes.

http://gerrit.ovirt.org/#/c/21244/9/tools/edit-node
File tools/edit-node:

Line 949:                             if "file://" in line:
Line 950:                                 self.item = re.match(r'^baseurl=file://(.*)', \
Line 951:                                                      line).group(1)
Line 952:                                 conf_builder += re.sub(r'(^baseurl=file://).*',
Line 953:                                                        r'\1/tmp/rpms',
Please use variables for paths.
Line 954:                                                        line)
Line 955:                             else:
Line 956:                                 conf_builder += line
Line 957:                 except:


Line 956:                                 conf_builder += line
Line 957:                 except:
Line 958:                     self.item = ""
Line 959:                 if os.path.exists(self.item):
Line 960:                     f = open("/tmp/plugin.repo", "w")
please use the with … construct

    with open(...) as src:
Line 961:                     f.write(conf_builder)
Line 962:                     f.close()
Line 963:                     options.repo = "/tmp/plugin.repo"
Line 964:                     os.system("mkdir %s/tmp/rpms" % self._instroot)


Line 1107:                           % self._instroot)
Line 1108:         if os.path.exists(self._instroot + "/tmp/yumrepo"):
Line 1109:             # cleanup local repo if exists
Line 1110:             if os.system("umount %s/tmp/yumrepo" % self._instroot):
Line 1111:                 os.system("rm -rf %s/tmp/yumrepo" % self._instroot)
please also use rmdir here.
Line 1112:         if os.path.exists(self._instroot + "/tmp/rpms"):
Line 1113:             # cleanup local rpms if exists
Line 1114:             if os.system("umount %s/tmp/rpms" % self._instroot):
Line 1115:                 os.system("rm -rf %s/tmp/rpms" % self._instroot)


Line 1111:                 os.system("rm -rf %s/tmp/yumrepo" % self._instroot)
Line 1112:         if os.path.exists(self._instroot + "/tmp/rpms"):
Line 1113:             # cleanup local rpms if exists
Line 1114:             if os.system("umount %s/tmp/rpms" % self._instroot):
Line 1115:                 os.system("rm -rf %s/tmp/rpms" % self._instroot)
you can use rmdir %s here
Line 1116:         if os.path.exists("/tmp/plugin.repo"):
Line 1117:             os.system("rm -rf /tmp/plugin.repo")
Line 1118:         if os.path.exists(self.dd_dir):
Line 1119:             os.system("umount %s" % self.dd_dir)


Line 1113:             # cleanup local rpms if exists
Line 1114:             if os.system("umount %s/tmp/rpms" % self._instroot):
Line 1115:                 os.system("rm -rf %s/tmp/rpms" % self._instroot)
Line 1116:         if os.path.exists("/tmp/plugin.repo"):
Line 1117:             os.system("rm -rf /tmp/plugin.repo")
no need for -r, only -f
Line 1118:         if os.path.exists(self.dd_dir):
Line 1119:             os.system("umount %s" % self.dd_dir)
Line 1120:             shutil.rmtree(self.dd_dir)
Line 1121:         return


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic44264f0c62c8785c0b4867fda1e45522d32fc1f
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: hadong <hadong0720 at gmail.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Itamar Heim <iheim at redhat.com>
Gerrit-Reviewer: Joey Boggs <jboggs 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