[ovirt-devel] [rhev-tech] Correct way to import hooking?

Tue Nov 28 23:38:21 UTC 2017

Hi Piotr,

Yes, I saw Nir's comment on the gerrit.
I'll leave my hook doing "import hooking" then.

Thanks a lot for the help.

On Tue, Nov 28, 2017 at 6:29 PM, Piotr Kliczewski <
piotr.kliczewski at gmail.com> wrote:

> On Mon, Nov 27, 2017 at 11:56 PM, Germano Veit Michel
> <germano at redhat.com> wrote:
> > Hi Piotr and Nir,
> >
> > Thanks for following up and sorry for sending to the wrong email list.
> >
> > So is the conclusion that 84638 will be merged and all hooks will just
> > "import hooking"?
>
> It is not easy to change it so we need to keep above import. I haven't
> seen any disagreement so I
> hope that the patch will be merged soon to enable you to write hook tests.
>
> >
> > Thanks,
> >
> >
> >
> > On Mon, Nov 27, 2017 at 6:32 PM, Piotr Kliczewski
> > <piotr.kliczewski at gmail.com> wrote:
> >>
> >> On Sat, Nov 25, 2017 at 1:56 AM, Nir Soffer <nsoffer at redhat.com> wrote:
> >> > On Fri, Nov 24, 2017 at 5:34 PM Piotr Kliczewski
> >> > <piotr.kliczewski at gmail.com> wrote:
> >> >>
> >> >> On Fri, Nov 24, 2017 at 4:00 PM, Piotr Kliczewski
> >> >> <piotr.kliczewski at gmail.com> wrote:
> >> >> > On Fri, Nov 24, 2017 at 2:20 PM, Nir Soffer <nsoffer at redhat.com>
> >> >> > wrote:
> >> >> >> Adding devel at ovirt, the proper mailing list
> >> >> >>
> >> >> >> בתאריך יום ו׳, 24 בנוב׳ 2017, 7:42, מאת Germano Veit Michel
> >> >> >> ‏<germano at redhat.com>:
> >> >> >>>
> >> >> >>> Hi,
> >> >> >>>
> >> >> >>> I'm trying to write a test for a hook. The test will fail if the
> >> >> >>> hook
> >> >> >>> does
> >> >> >>> "import hooking", which seems to be the norm for vdsm hooks.
> >> >> >>>
> >> >> >>> [vdsm_hooks]$ grep -rn "import hooking" | wc -l
> >> >> >>> 55
> >> >> >>>
> >> >> >>> Not sure if I'm doing something wrong, but it looks like I would
> >> >> >>> need
> >> >> >>> vdsm
> >> >> >>> installed on my development machine in order for this import to
> >> >> >>> work.
> >> >> >>>
> >> >> >>>
> >> >> >>> ============================================================
> ==========
> >> >> >>> ERROR: test suite for <class
> >> >> >>> 'virttests.boot_hostdev_test.BootHostdevHookTests'>
> >> >> >>>
> >> >> >>> ------------------------------------------------------------
> ----------
> >> >> >>> Traceback (most recent call last):
> >> >> >>>   File "/usr/lib/python2.7/site-packages/nose/suite.py", line
> 209,
> >> >> >>> in
> >> >> >>> run
> >> >> >>>     self.setUp()
> >> >> >>>   File "/usr/lib/python2.7/site-packages/nose/suite.py", line
> 292,
> >> >> >>> in
> >> >> >>> setUp
> >> >> >>>     self.setupContext(ancestor)
> >> >> >>>   File "/usr/lib/python2.7/site-packages/nose/suite.py", line
> 315,
> >> >> >>> in
> >> >> >>> setupContext
> >> >> >>>     try_run(context, names)
> >> >> >>>   File "/usr/lib/python2.7/site-packages/nose/util.py", line
> 471,
> >> >> >>> in
> >> >> >>> try_run
> >> >> >>>     return func()
> >> >> >>>   File
> >> >> >>>
> >> >> >>>
> >> >> >>> "/home/gveitmic/Source/upstream/vdsm/tests/virttests/
> boot_hostdev_test.py",
> >> >> >>> line 127, in setUpClass
> >> >> >>>     import before_vm_start as boot_hostdev_hook
> >> >> >>>   File "../vdsm_hooks/boot_hostdev/before_vm_start.py", line
> 24, in
> >> >> >>> <module>
> >> >> >>>     import hooking
> >> >> >>> ImportError: No module named hooking
> >> >
> >> >
> >> > This is expected, hooking is not install in the pyhton path by
> default.
> >> >
> >> > All the hooks are using wrong import. Vdsm is "fixing" the bad
> >> > import by modifying the python path.
> >> >
> >> >>
> >> >> >>>
> >> >> >>> I can make it go away by using this in my hook:
> >> >> >>>
> >> >> >>> from vdsm.hook import hooking
> >> >
> >> >
> >> > This is correct import - I would use this.
> >> >
> >> >>
> >> >> >>>
> >> >> >>> Then the test succeeds fine. But then I see all hooks use "import
> >> >> >>> hooking"
> >> >> >>> instead of "from vdsm.hook import hooking".
> >> >> >>>
> >> >> >>> [vdsm_hooks]$ grep -rn "from vdsm.hook import hooking" | wc -l
> >> >> >>> 0
> >> >> >>>
> >> >> >>> Am I doing something wrong? Can someone put a light here? What is
> >> >> >>> the
> >> >> >>> correct way to do this import?
> >> >> >
> >> >> > Our approach for it is to modify PYTHONPATH [1] where we add
> >> >> > vdsm.hook.
> >> >> > It looks like we do not have similar update in [2] - script which
> run
> >> >> > the tests.
> >> >> >
> >> >> > I will push a patch to fix it.
> >> >>
> >> >> https://gerrit.ovirt.org/#/c/84638/
> >> >>
> >> >> Please check whether it would work for you now.
> >> >
> >> >
> >> > This can be handy for testing legacy hooks, but I think we should
> >> > fix the imports instead, so no manipulation of python path is needed.
> >> >
> >>
> >> I agree that the path is not perfect way of doing it. I would love to
> fix
> >> it but
> >> please note that people use `import hooking` in their own custom hooks
> >> and we are unable to change them. This means that we won't be able to
> >> fix it easily if ever. Because of that I am not sure whether it is good
> >> idea to
> >> introduce different way of importing the module.
> >>
> >> Till now we haven't had hook tests so the module was not available on
> the
> >> path.
> >>
> >> >>
> >> >>
> >> >> >
> >> >> > [1] https://github.com/oVirt/vdsm/blob/master/lib/vdsm/hooks.py#
> L98
> >> >> > [2]
> >> >> >
> >> >> > https://github.com/oVirt/vdsm/blob/master/tests/run_tests_
> local.sh.in#L21
> >> >> >
> >> >> >>>
> >> >> >>> Thanks,
> >> >> >>> Germano
> >> >> >>
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> Devel mailing list
> >> >> >> Devel at ovirt.org
> >> >> >> http://lists.ovirt.org/mailman/listinfo/devel
> >
> >
> >
> >
> > --
> > Germano Veit Michel <germano at redhat.com>
> > Senior Software Maintenance Engineer, Virtualization, Red Hat
>

-- 
Germano Veit Michel <germano at redhat.com>
Senior Software Maintenance Engineer, Virtualization, Red Hat
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20171129/281fb18b/attachment.html>