Checkbox Provider - Base

Code review comment for ~bladernr/plainbox-provider-checkbox:1628264-virt-timeout

Revision history for this message

Jeff Lane  (bladernr) wrote on 2017-06-22:

On Wed, Jun 21, 2017 at 12:29 PM, Rod Smith <email address hidden> wrote:
> The Bad
> -------
>
> Both "pylint" and "flake8" produce large numbers of errors and warnings on this script. Most of these are for existing code, so it's unfair to ask you to fix them on this submission; however, there were a few complaints about code
> submitted here. From the pylint output:

There's a reason I don't use pylint and flake8. Pylint, for example,
also tells me this:

C:742, 0: Unnecessary parens after 'print' keyword (superfluous-parens)
C:745, 0: Unnecessary parens after 'print' keyword (superfluous-parens)

which is incorrect, python3 treats print as a function and the parens
are passing arguments to that function... they're both a bit crazy
and require a good bit of configuration and work to produce useful
output, IMO. IN any case, I always check to the level of pep8, (apt
install pep8) which catches most of the PEP8 violations (the important
ones at least).

> C:443, 0: Exactly one space required after comma
> def log_check(self,stream):
> ^ (bad-whitespace)
> C:499, 0: Exactly one space required around assignment
> self.elapsed_time=0
> ^ (bad-whitespace)
> W:529,30: Use % formatting in logging functions and pass the % parameters as arguments (logging-format-interpolation)
> W:499,16: Attribute 'elapsed_time' defined outside __init__ (attribute-defined-outside-init)
>
> If you prefer to file a bug report about the large number of complaints by these tools and leave these as-is, I'm willing to let this slide, since they seem to be trivial things that don't affect the program's ability to work, and they're a drop in the bucket amidst the many similar complaints by pylint and flake8.

I did a few, just to see how bit a task it would be, and it would be
huge. If it really bugs you, we can revisit it later to pylint/flake8
it, but once you go down that road, it'll need to be done for every
python script in there ;) pep8 was the recommended tool way back
when, and afaik, still is, and most, or all, the scripts at this point
will pass a pep8 check.

> The Question:
> -------------
>
> Finally, concerning the while/else clause on lines 501-524: It seems that a lot of code is duplicated from the main while loop in the else clause. The main difference seems to be that, in the else clause, the output of debug_file.read() is
> saved to the variable stream so that it can be logged if there's an error. Wouldn't it make more sense to simply store output in the stream variable in the main while loop and then log it only if there's an error? If there's some reason this
> doesn't work, then I'll pass it as-is; but if I'm not overlooking something, it appears that the code could be made cleaner by revising this logic.

That's a good question, and I did spend a fair bit of time wrestling
with how to do what I wanted to do. In the end, this was the least
complicated... The file reads have to happen within the while loop,
since we read the contents and close the filehandle on each pass. I
didn't want to open a second filehandle (since KVM is already writing
to that file) that was continuously open. And the only time we hit
the Else is IF the boot success message has not appeared before
elapsed_time > self.timeout, meaning we've elapsed our timeout setting
without getting the success message.

And once the Else is hit, the first part of that loop is bypassed, so
the file is not read without including a second read. So I had to put
the second open bit in there. I capture the output this time because
if THIS is hit, AND the success message isn't in the log, we need to
error and dump the log. But in the success case, we don't need the
log output at all. We don't need to dump it, it's already attached in
a later virt_debug-attach job that runs after the KVM test runs.

I guess, ultimately, it's checking for a positive, not a negative.
the "error" is that the message CERTIFICATION BOOT COMPLETE has not
appeared in the console log.

At least, that's how it worked out in my mind. There may be a more
complex way to do it, but that was clean and does the job.

On Wed, Jun 21, 2017 at 12:29 PM, Rod Smith <rod.smith@canonical.com> wrote:
> The Bad
> -------
>
> Both "pylint" and "flake8" produce large numbers of errors and warnings on this script. Most of these are for existing code, so it's unfair to ask you to fix them on this submission; however, there were a few complaints about code
> submitted here. From the pylint output:

There's a reason I don't use pylint and flake8.  Pylint, for example,
also tells me this:

C:742, 0: Unnecessary parens after 'print' keyword (superfluous-parens)
C:745, 0: Unnecessary parens after 'print' keyword (superfluous-parens)

which is incorrect, python3 treats print as a function and the parens
are passing arguments to that function...  they're both a bit crazy
and require a good bit of configuration and work to produce useful
output, IMO.  IN any case, I always check to the level of pep8, (apt
install pep8) which catches most of the PEP8 violations (the important
ones at least).

> C:443, 0: Exactly one space required after comma
>     def log_check(self,stream):
>                       ^ (bad-whitespace)
> C:499, 0: Exactly one space required around assignment
>                 self.elapsed_time=0
>                                  ^ (bad-whitespace)
> W:529,30: Use % formatting in logging functions and pass the % parameters as arguments (logging-format-interpolation)
> W:499,16: Attribute 'elapsed_time' defined outside __init__ (attribute-defined-outside-init)
>
> If you prefer to file a bug report about the large number of complaints by these tools and leave these as-is, I'm willing to let this slide, since they seem to be trivial things that don't affect the program's ability to work, and they're a drop in the bucket amidst the many similar complaints by pylint and flake8.

I did a few, just to see how bit a task it would be, and it would be
huge.  If it really bugs you, we can revisit it later to pylint/flake8
it, but once you go down that road, it'll need to be done for every
python script in there ;)  pep8 was the recommended tool way back
when, and afaik, still is, and most, or all, the scripts at this point
will pass a pep8 check.

That's a good question, and I did spend a fair bit of time wrestling
with how to do what I wanted to do.  In the end, this was the least
complicated...  The file reads have to happen within the while loop,
since we read the contents and close the filehandle on each pass.  I
didn't want to open a second filehandle (since KVM is already writing
to that file) that was continuously open.  And the only time we hit
the Else is IF the boot success message has not appeared before
elapsed_time > self.timeout, meaning we've elapsed our timeout setting
without getting the success message.

And once the Else is hit, the first part of that loop is bypassed, so
the file is not read without including a second read.  So I had to put
the second open bit in there.  I capture the output this time because
if THIS is hit, AND the success message isn't in the log, we need to
error and dump the log.  But in the success case, we don't need the
log output at all.  We don't need to dump it, it's already attached in
a later virt_debug-attach job that runs after the KVM test runs.

I guess, ultimately, it's checking for a positive, not a negative.
the "error" is that the message CERTIFICATION BOOT COMPLETE has not
appeared in the console log.

At least, that's how it worked out in my mind.  There may be a more
complex way to do it, but that was clean and does the job.

« Back to merge proposal