Code review comment for lp:~justin-fathomdb/nova/raw-disk-images

Revision history for this message
termie (termie) wrote :

I'll just weigh in here on my thoughts about Cheetah, mainly that I think it is bad to allow much logic in the template code. Agreed that we all need conditionals, but seeing 'getVar' in there really throws some flags for me.

A much much smaller but powerful and declarative-style templating engine that I've grown to prefer is called jsontemplate, http://json-template.googlecode.com/svn/trunk/doc/Introducing-JSON-Template.html

Our usage is pretty minimal here so the syntax isn't shockingly different but in Cheetah

#if $getVar('kernel', None)
        <kernel>${kernel}</kernel>
    #if $getVar('ramdisk', None)
        <initrd>${ramdisk}</initrd>
    #end if
        <cmdline>root=/dev/vda1 console=ttyS0</cmdline>
#end if

and

template_contents = open(FLAGS.libvirt_xml_template).read()
str(Template(template_contents, searchList=[ xml_info ] ))

becomes

{.section kernel}
        <kernel>{kernel}</kernel>
    {.section ramdisk}
        <initrd>{ramdisk}</initrd>
    {.end}
        <cmdline>root=/dev/vda1 console=ttyS0</cmdline>
{.end}

and

jsontemplate.FromFile(FLAGS.libvirt_xml_template).expand(xml_info)

The other benefits are that the engine is a single file and easily extensible with formatters

The "json" part of it comes from the fact that it was designed and implemented in python and javascript at the same time.

There are a variety of style nits (extra spaces before or around things, importing objects instead of modules, using tabs instead of spaces in the xml), so please take another scan over the code to resolve those :)

Other than those discussion points looks like a great addition to our core functionality :)

review: Needs Fixing

« Back to merge proposal