[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 11: Code-Review-1

(4 comments)

Just some nitpicks, then fine

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

Line 956:                     conf_builder = ""
Line 957:                     rpms_path = "/tmp/rpms"
Line 958:                     with open(options.repo, "r") as f:
Line 959:                         for line in f.readlines():
Line 960:                             if "file://" in line:
better to use line.startswith here
Line 961:                                 self.item = re.match(r'^baseurl=file://(.*)', \
Line 962:                                                      line).group(1)
Line 963:                                 conf_builder += re.sub(r'(^baseurl=file://).*',
Line 964:                                                        r'\1' + rpms_path,


Line 957:                     rpms_path = "/tmp/rpms"
Line 958:                     with open(options.repo, "r") as f:
Line 959:                         for line in f.readlines():
Line 960:                             if "file://" in line:
Line 961:                                 self.item = re.match(r'^baseurl=file://(.*)', \
We will miss lines whic huse i.e.: baseurl = file://… or mirrorurl = file://

Please enhance the expr to also cover mirrorurls and spaces

i.e.:
[^=]+=\s+file://

or so
Line 962:                                                      line).group(1)
Line 963:                                 conf_builder += re.sub(r'(^baseurl=file://).*',
Line 964:                                                        r'\1' + rpms_path,
Line 965:                                                        line)


Line 959:                         for line in f.readlines():
Line 960:                             if "file://" in line:
Line 961:                                 self.item = re.match(r'^baseurl=file://(.*)', \
Line 962:                                                      line).group(1)
Line 963:                                 conf_builder += re.sub(r'(^baseurl=file://).*',
smae as above
Line 964:                                                        r'\1' + rpms_path,
Line 965:                                                        line)
Line 966:                             else:
Line 967:                                 conf_builder += line


Line 964:                                                        r'\1' + rpms_path,
Line 965:                                                        line)
Line 966:                             else:
Line 967:                                 conf_builder += line
Line 968:                 except:
please log the exception somewhere maybe even with log.debug(…, exc_info=True) to not print it on the screen
Line 969:                     self.item = ""
Line 970:                 if os.path.exists(self.item):
Line 971:                     with open("/tmp/plugin.repo", "w") as f:
Line 972:                         f.write(conf_builder)


-- 
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: 11
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