Okay, I'll resend the patch fixing Michele's remarks.
On Thu, Apr 4, 2013 at 11:19 AM, Bernardo Dal Seno <bdals...@google.com>wrote: > On 4 April 2013 10:03, Helga Velroyen <hel...@google.com> wrote: > > Hi! > > > > > > On Wed, Apr 3, 2013 at 6:19 PM, Bernardo Dal Seno <bdals...@google.com> > > wrote: > >> > >> On 28 March 2013 15:50, Helga Velroyen <hel...@google.com> wrote: > >> > This patch is a preparation for later patches in QA in this series > >> > it refactors the instance QA to make it more flexible regarding > >> > creation of instances with different disk templates. Right now we > >> > only support creation of instances with disk templates 'drbd', > 'plain', > >> > and 'diskless'. With our current plans to improve storage handling, > >> > we should make QA more flexible to also create instances of other > >> > templates. This patch restructures the code in a way that instances > >> > can be created more easily to be used in other QA scripts. > >> > > >> > Signed-off-by: Helga Velroyen <hel...@google.com> > >> > --- > >> > qa/qa_instance.py | 88 > >> > ++++++++++++++++++++++++++++++++++++++++++++++++++----- > >> > 1 file changed, 80 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/qa/qa_instance.py b/qa/qa_instance.py > >> > index f1c2ab7..9ca36eb 100644 > >> > --- a/qa/qa_instance.py > >> > +++ b/qa/qa_instance.py > >> > @@ -93,6 +105,68 @@ def _DiskTest(node, disk_template, fail=False): > >> > return None > >> > > >> > > >> > +def _CreateInstanceByDiskTemplateOneNode(nodes, disk_template, > >> > fail=False): > >> > + """Creates an instance using the given disk template for disk > >> > templates > >> > + for which one given node is sufficient. These templates are for > >> > example: > >> > + plain, diskless, file, sharedfile, blockdev, rados. > >> > + > >> > + @type nodes: list of nodes > >> > + @param nodes: a list of nodes, whose first element is used to > create > >> > the > >> > + instance > >> > + @type disk_template: string > >> > + @param disk_template: the disk template to be used by the instance > >> > + @return: the created instance > >> > + > >> > + """ > >> > + assert len(nodes) > 0 > >> > >> Why not "assert len(nodes) == 1"? > > > > > > Because to keep it more flexible. It should be possible to give it more > than > > one node and still work properly. The function just uses the first one > and > > ignores the rest. This was more elegant when testing all disk templates > and > > calling the "CreateInstanceByDiskTemplate" function which then selects > the > > correct subfunction (of which is one this > > I see your point. Still I don't like when a function silently ignores > its input, as too often that hides an error, but since you've already > used this feature, I'd say it's okay to leave it this way. > > Thanks, > Bernardo >
