Ondřej Svoboda has posted comments on this change.
Change subject: netinfo: Do not mix local time and UTC, improve tests ......................................................................
Patch Set 4:
(2 comments)
http://gerrit.ovirt.org/#/c/24192/4/tests/functional/networkTests.py File tests/functional/networkTests.py:
Line 1702: Line 1703: with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir: Line 1704: confFile = os.path.join(tmpDir, 'test.conf') Line 1705: leaseFile = os.path.join(tmpDir, 'test.lease') Line 1706: pidFile = os.path.join(tmpDir, 'test.pid')
In a follow-up patch, please generate pidFile within dhcp.runDhclient() - w
dhcp.runDhclient() is tightly bound to its only caller. If pidFile did not matter, I would make it an optional argument. But it is important to avoid a possible collision with another dhclient. If it was running, our own might refuse to run. Isolating any files dhclient uses is a good idea and working with them in one place is good for consistency.
Not passing the file name would require passing the path anyway. The minimum would be to pass the working path and the path to the lease file. Or refactoring the whole: move creating a tmpdir to dhcp.runDhclient and making it return a path to a lease file. But how would I clean up? Line 1707: Line 1708: with open(confFile, 'w') as f: Line 1709: f.write('db-time-format {0};'.format(dateFormat)) Line 1710:
http://gerrit.ovirt.org/#/c/24192/4/tests/netinfoTests.py File tests/netinfoTests.py:
Line 301: ' interface "valid";\n' Line 302: ' expire {0:%w %Y/%m/%d %H:%M:%S};\n' Line 303: '}}\n' Line 304: 'lease {{\n' # in an actual lease file, Line 305: ' interface "valid2";\n' # human-readable date follows:
but to simulate an actual lease file, this comment should be inside the str
Would it be acceptable to include these fixed comments in the string and make a comment outside explaining that they are mere examples?
An explanation is needed in any case, even when I would put real-looking locale-aware dates in comments, this time to say they aren't actually used...
I commented on the previous patch that the comments are not meant for a machine anyway...
Quoting man 5 dhclient.conf:
"The # symbol supplies a comment that describes what actual time this is as according to the system's configured timezone, at the time the value was written. It is provided only for human inspection, the epoch time is the only recommended value for machine inspection."
"# %a %b %d %H:%M:%S %Y" is used as a format string in dhclient's source code. Line 306: ' expire epoch {1:.0f};\n' Line 307: # expire epoch 2117041460; # Sat Jan 31 20:04:20 2037 Line 308: '}}\n' Line 309: 'lease {{\n'