opencompute-developers team mailing list archive

[Jeff Lane <jeffrey.lane@xxxxxxxxxxxxx>]

Review: Disapprove

Hi Nelson...

I really hate to do this, but I'm going to reject this Merge Proposal.  Here's why:

Please, PLEASE make several, smaller, targeted merge requests rather than one large 1500 line merge.  The only time a merge should have that many lines of changed code is if it adds or changes a very large chunk of code, not adding so many small, individual files.

Smaller merge requests make it a lot easier to review, make suggestions and get things merged in a more timely fashion.

I would suggest breaking this merge request up into smaller chunks divided based on testing topic.  So, for example submit one merge request with your proposed changes for:
TC-001-0001-CPU_Memory
whcih should include the whitelist changes, the added scripts, added/modified job files, etc.

Then do the same in different merge requests for:
TC-001-0002-Platform_Controller_Hub
TC-001-0003-BIOS_setup_menu
TC-001-0004-BIOS_remote_update
TC-001-0005-BIOS_event_log
and so on.

Now, a word about job naming and whitelists that apply across all these changes:

1:  Dont put whitespace (space, tab) in the job name.
    Rather than TC-001-0001-001 CPU information, you should use something like these:
        TC-001-0001-001-CPU_information
        TC-001-0001/001_CPU_information
2:  I don't know that it will hurt, but try to remove any blank lines in the whitelist file, the sections are identifiable by the __SUITENAME__ items on the list, so blank lines are not necessary.  
3:  You CAN, however, add comments preceding them with the # character
4:  remember that every job called __SUITENAME__ must exist in the local.txt.in file (see that file for examples)
    That job is a special job that causes the suitename.txt.in file to be read in when checkbox launches.

Beyond those suggestions, We'll deal with individual problems in the smaller merge requests I've asked for.  

Also, given that you are new to this process, using smaller merge requests will make it a LOT easier for me to help show you through the process as well as acquaint you with the way checkbox works and how the code needs to look.

-- 
https://code.launchpad.net/~nelson-chu/opencompute/add-ocp-certified-jobs/+merge/196476
Your team Open Compute Developers is subscribed to branch lp:opencompute/checkbox.