cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #00347
Re: [Merge] lp:~harm-o/cloud-init/freebsd into lp:cloud-init
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.
Follow ups
References