2013/2/12 Michele Tartara <[email protected]>:
> lib/backend.py | 46
> ++++++++++++++++++++++++++++++++++++++++
> lib/cli.py | 5 +++++
> lib/constants.py | 9 ++++++++
> lib/pathutils.py | 16 ++++++++++++++
> lib/tools/ensure_dirs.py | 2 ++
> src/Ganeti/OpParams.hs | 10 +++++++++
> src/Ganeti/Types.hs | 10 +++++++++
> test/hs/Test/Ganeti/OpCodes.hs | 2 ++
Where are the unittests for the Python code?
> --- a/lib/backend.py
> +++ b/lib/backend.py
> +class InstReason(object):
> + """Class representing the reason for a change of state of a VM.
> +
> + It is used to allow an easy serialization of the reason, so that it can be
> + written on a file.
> +
> + """
> +
> + def __init__(self, text, source):
There should not be an empty line before the first method (or member
in general) in a class.
> + """Initialize the class with all the required values.
> […]
> + def GetJson(self):
> + """Get the JSON representation of the InstReason
> +
> + @rtype: string
> + @return : The JSON representation of the object.
> +
> + """
> + fields = ["text", "source"]
> + as_dict = dict((field, getattr(self, field)) for field in fields)
Why don't you just write it as follows?
return serializer.DumpJson(dict(text=self.text, source=self.source))
> + return serializer.DumpJson(as_dict)
> […]
> --- a/lib/pathutils.py
> +++ b/lib/pathutils.py
> +def GetInstReasonFilename(instance_name):
> + """Returns the full path for the file name containing the reason of the
> + last status change of an instance.
The first line should not be wrapped. Put the rest in a paragraph.
> +
> + @type instance_name: string
> + @param instance_name: The name of the instance.
No punctuation, please (might apply elsewhere too).
> + @rtype: string
> + @return: The path of the file.
See above.
> +
> + """
> + return os.path.join(INSTANCE_REASON_DIR, instance_name)
Now that I see this, I think you should put this function into
backend.py or utils/io.py. The reason is that you should use
utils.PathJoin. “os.path.join”' behaviour when an argument is absolute
or contains slashes (os.path.join("/tmp", "/bin/ls")) is undesired. If
someone manages to inject an instance name with a slash the code might
end up overwriting a file outside of INSTANCE_REASON_DIR.
Michael
