cloud-init-dev team mailing list archive

[Joshua Harlow <harlowja@xxxxxxxxx>]

Great start!

Some initial comments :-)

givecmdline()
  - can this function be in the distro object itself??
  - can we use signal.SIG_DFL instead of just 0 (using the constant makes it so people can lookup that on websites, and understand it a little easier)
  - sys.platform -> util.system_info(), this little util function should be helpful here, its using something that can be mocked out (and uses the platform module instead of sys module)

freebsd.py
 - for rc.conf have u tried the parsers/sys_conf.py, it might be more robust than the one u created (from initial looking at, it seems like it should work, might be worth a try)

utils.py
 - an idea, maybe for a future change, is to have a parser for the mount data, similar to the ones we have in https://bazaar.launchpad.net/~cloud-init-dev/cloud-init/trunk/files/head:/cloudinit/distros/parsers/, that way its pretty easy to decouple the getting of the mount data from the parsing of it (allowing the parsing to be tested in a way that is not connected to the executing)
 - logexc(LOG, "Failed fetching mount points"), can we also include the method that was used here, this will help in case of debugging why this method may have failed, --> logexc(LOG, "Failed fetching mount points using method %s", method)
 - wow, didn't know uptime had to be done with ctypes, is there no other way? is the `uptime` command pretty standard? for the logexc(LOG, "Unable to read uptime") can we also add in the method (similar to the above mount method); so that if it fails we can easily know which method was tried, maybe we could consider using https://pypi.python.org/pypi/uptime which seems to be more platform agnoistic (and simpler to use) - http://pythonhosted.org/uptime/#supported-platforms
 - /etc/mtab parsing, seems like another good candiate for beign a parser in https://bazaar.launchpad.net/~cloud-init-dev/cloud-init/trunk/files/head:/cloudinit/distros/parsers/ thoughts?


-- 
https://code.launchpad.net/~harm-o/cloud-init/freebsd/+merge/198130
Your team cloud init development team is requested to review the proposed merge of lp:~harm-o/cloud-init/freebsd into lp:cloud-init.