List Of Improvements

Comments in the Patch
Patch Set 40:

(16 comments)

I reviewed the first 5 files for now. [Documentation] is missing in most (if not all) keywords. Cool stuff. looking forward to seeing how it looks in the sandbox and how much time we might save during setup.
Commit Message
File Comment
need a better commit message. I think this patch is large enough that some good details in the commit messages is warranted
csit/libraries/OVSDB.robot
Line 203:
don't need ALL CAPS, and (for me) I don't know what Alt Approach really means. Is it not possible to use the existing keyword to get UUID? or enhance it?
Line 207:
double check if needed, but I think the RARRAO keyword in line 206 above will give us the output for free. no need to add extra logs to the already giant output.xml
Line 208:
Sam showed me this:

Should Be True '${rc}' == '0'

I think that's more clear. If you agree, let's use that. I'll try to re-factor this in other places that already exist.
Line 211:
I think 'on' is better than 'to'

same below for the other config keyword
Line 217:
don't think we need it.
Line 222:
ws
Line 227:
don't need it, I think.
csit/libraries/OpenStackInstallUtils.robot
Line 40:
probably can be a little more clean and scalable if we use loops.

psuedo code:

for i in num_control_nodes:
${control_${i}_connection}= Get Ssh Connection ...
Set Suite Variable ${control_${i}_connection}

then the same for compute nodes
Line 41:
ws
csit/libraries/OpenStackOperations.robot
Line 140:
don't need Log and change Should Not Be True to

Should Be True '${rc}' == '0'

this applies in multiple places, so I wont make it a comment again.
Line 1106:
ws
csit/libraries/OpendaylightInstallUtils.robot
Line 13:
maybe a better name is "Configure Boot Features"?
Line 16:
ws
Line 17:
interesting. does this work? does it overwrite the default featuresBoot features that are there?
Line 23:
ws

Can we add more details in the commit message on how this works? Biggest confusion I have is this looks to be doing some of the work that the builder side was doing, so not sure what the work split is now.
csit/libraries/OpenStackInstallUtils.robot
Line 2:
this comment should be more about openstack install utilities
Line 10:
would this stuff be better to add to DevstackUtils?
Line 16:
temporarily

Does selinux really need to be disabled or do we need to open something up? I thought we fixed everything in builder to open up the right access.
Line 21:
DevstackUtils.Devstack Suite Setup has code to connect to the nodes. could that be used?
Line 41:
is this a standard prompt defined somewhere already? And password. Maybe they should at least be defaults on the Setup Basic Ssh keyword.
csit/libraries/OpenStackOperations.robot
Line 143:
everything is now wrapped with the OpenStack CLI keyword
Line 332:
this is deleted now - do you need it?
Line 457:
do we need this extra log - the command logs the fip already?
csit/libraries/OpendaylightInstallUtils.robot
Line 2:
odl comment
csit/suites/netvirt/install/deploy_odl_with_netvirt.robot
Line 2:
all
Line 24:
whitespace
csit/suites/openstack/connectivity/external_network.robot
Base, Line 114:
does this need to be removed? Is it replaced with a different test?