Closed gjim83 closed 7 years ago
@gjim83 Nice catch...can you submit a pull-request on this into the NAPALM-panos driver.
@ktbyers sure thing, was just waiting on comments. However, convert_uptime_string_seconds()
is in napalm_base/utils/string_parsers.py so I presume you mean submit the PR here? :)
Yep...first screw up today, I am confident more will follow :-)
So the problem I see is that your solution assumes "0 day(s)" is always there and I don't know how universal this is across different platforms.
I don't really like the code we have here as it is fragile.
regex1 = (r"((?P<weeks>\d+) week(s)?,\s+)?((?P<days>\d+) day(s)?,\s+)?((?P<hours>\d+) "
r"hour(s)?,\s+)?((?P<minutes>\d+) minute(s)?)")
regex_1 = re.compile(regex1)
regex_2 = re.compile(r"((?P<hours>\d+)):((?P<minutes>\d+)):((?P<seconds>\d+))")
regex3 = (r"((?P<weeks>\d+)w)?((?P<days>\d+)d)?((?P<hours>\d+)h)?"
r"((?P<minutes>\d+)m)?((?P<seconds>\d+)s)?")
regex_3 = re.compile(regex3)
regex_list = [regex_1, regex_2, regex_3]
We also need to add your case into the unit tests so that it catch this failure.
So the problem I see is that your solution assumes "0 day(s)" is always there and I don't know how universal this is across different platforms.
I'm actually ?
-catching the X days,
output,
((?P<days>\d+) day(s)?,\s+)?
so when the time stamp for a different platform is just hh:mm:ss
it'll still get caught.
I agree on the code being a bit brittle however I'm not sure how to make it more stable without knowing all possible formats. E.g. I can tell that PanOS doesn't state weeks or months, but I couldn't find a firewall in our fleet that's been up over a year to see if it keeps counting >365 days or if switches to 1 year(s)? 0 days...
I wrote code for parsing uptime strings a while back (where I had a quite a few different uptime strings). Let me look at what I did there.
Key thing is the unit tests i.e. that we are feeding enough varied output strings through the unit tests that we properly catch errors (and then expand the unit tests if we missed some).
Absolutely. Will start working on them soon and post the PR as soon as I think it's ready.
Simple issue. Up time has the following format
114 days, 22:27:32
("days" is always there, for <24 hours it shows0 days,
). None of the current regexes capture the whole string.The above up time was close to the actual one when I called
get_facts()
, and the response was'uptime': 80847
which as you can tell doesn't take into account the 114 days (plus a few seconds difference as the raw string comes from directly checking the CLI).Locally I changed:
to:
So with the same string now the up time is correctly calculated as
27210452