Re: [libvirt] [test-API PATCH 5/6] Cleanup and fix of domain/define test

On 03/27/2012 05:42 PM, Martin Kletzander wrote: > On 03/27/2012 10:59 AM, Guannan Ren wrote: >> On 03/27/2012 03:57 PM, Martin Kletzander wrote: >>>> It's better not to use libvirt API to check the result >>>> of one >>>> another API. >>>> We should use native approach to do the checking. So I >>>> insist >>>> on the original checking. >>>> >>> There was no lookupByName function, but I agree it's better to use the >>> same approach, so I'll add this method to the connection API. >>> One more question, whilst on this subject, though. I still didn't get >>> why we encapsulate libvirt API into one more class where we don't make >>> use of any persistence, inheritance nor any other OOP approach. It >>> would >>> help me to understand so I don't make future mistakes. >> I think you don't need to add lookupByName() in connectAPI.py >> The APIs is domain related, so we suggest to make use of it in >> domainAPI.py >> only. The ideas is that all of hypervisor related APIs go to >> connectAPI.py. >> The relationship between classes domain, network, storage, >> nodedev is >> loose. If we had a virtnetwork subclass, It would be better to make >> virtnetwork inherite >> network for better OOP. >> The encapsulated API files in lib directory is easy to manage >> and use. For example >> If we want to write a domain testcases, we probably don't need to >> import network >> and storage module in lib. >> > We both probably talk about something else, let me clarify. > > My apologies first, because I misunderstood that you wanted to use the > native approach. I thought you meant implementing the function in > connectAPI whether you meant checking on the machine without using > libvirt. That said, there is no point in talking about lookupByName > implementation for now. > > About the actual checking if the domain was created. Leaving it as it is > now, any misconfiguration will make this check fail (meaning the > configuration of sshd, libvirt, etc.). It will work for now as we are > the only ones using that right now (I guess), I'm just trying to think > ahead and I don't see that big problem with checking the domain being > created using libvirt again, but that's just my opinion. > > About the re-implementation of the API, I was just looking into the > connectAPI class for now, but what I see there is only constructor that > saves the connection from libvirt and then for each method of libvirts > connection, there is connectAPI method that does the same, just using > different method names (and raising different exception). Unfortunately > these aren't even consistent. For example: > > libvirts openAuth is encapsulated as openAuth > libvirts isEncrypted is encapsulated as isEncrypted > > but > > libvirts getCapabilities is encapsulated as get_caps > libvirts getHostname is encapsulated as get_host_name > > and so on. > > Don't get me wrong, I'm not trying to complain or anything, I'm just > trying to understand because for me it doesn't make any sense to use > this class. Ok, let me keep it temporarily, because it doesn't affect testing. After some practice, it proves to be useless, we can remove them anytime. Is that okay?