opencompute-developers team mailing list archive

[Jeff Lane <jeffrey.lane@xxxxxxxxxxxxx>]

Review: Needs Fixing

Hello Sophia!  Thank you for submitting this test case.  As these early cases are the first for many of you, I expect to do a lot of back and forth to help you all learn the process and make sure everything is correct before merging new code into checkbox-ocp.

So with that in mind, please correct the following things for the files you've modified or added:
jobs/local.txt.in:
* Nothing to do here, your local job description looks great!

jobs/TC-002-0011-System_Log.txt.in
* The job itself looks fine, but could you please clean up the description field?
  * Each description item should be on a separate line, indented by one space, like so:

description:
 1: item 1
 2: item 2
 3: item 3

  * The items themselves need to be a little more descriptive so that someone reading the job file will understand whats going on.  "1: dcmitool" really has no useful meaning, for example.

data/whitelists/opencompute-certify-local.whitelist:
* Nothing to do here, your whitelist looks perfect!

scripts/dcmi_sel_entries 
* in main(), please don't explicitly call sys.exit().  it's cleaner, since main() is a function, to use return statements and then have it called like so:

if __name__ == '__main__':
    sys.exit(main())

I know both ways work, but its more pythonic to have functions return values rather than explicitly exit

* There's no need for the multiple different return/exit codes.  In a different setting, you may want that, but checkbox only really sees 0 and non-0 exit codes.  Anything that returns 0 is a passing test, anything that returns non-0 is a Failing test and checkbox discards the codes at that point.  So an exit of 20 is exactly the same as an exit of 1 or an exit of -1.
* Line 52 should have 'Reason: %s', not 'Reason:%s' to make the output more clean.
* in the second try: block that calculates entry_space, you catch ArithmeticError... I would prefer if you also printed out the numbers being used for arithmetic, perhaps like this:

entry_used = int(get_entry_used(dcmi_sel_return))
entry_percent = int(get_entry_per(dmci_sel_return))
try:
        entry_space = entry_used / entry_percent * 100
except ArithmeticError:
        print("Arithmetic Error: ")
        print("entry_used: %s         entry_per: %s" % (entry_used, entry_per))
        return 1

That makes the output in a failure more useful and informative and reduces debugging time.
* speaking of which, you don't have to save characters with variable names, please make the variables more descriptive.  entry_per is not as descriptive as entry_percent_used, and the function name get_entry_per() is not as descriptive as get_entry_percent_used()
* line 64: print("Entries Number:", int(entry_space), "Entries Number is too least.")
   *"Entries Number is too least." should be "Entries Number is too low."
   *also "Entries Number:" needs a space between : and " to format the output better.  
   * I'm not positive int(entry_space) will work, because I am not sure int(get_entry_per(dcmi_sel_return)) will return something that can be cast as an int().  Can you provide me the raw output from "dcmitool sel"??


-- 
https://code.launchpad.net/~sophia-wu/opencompute/add-ocp-system-log-job/+merge/197006
Your team Open Compute Developers is subscribed to branch lp:opencompute/checkbox.