Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pybind/mgr: new 'hello world' mgr module skeleton #19491

Merged
merged 1 commit into from Apr 26, 2018

Conversation

Projects
None yet
6 participants
@yaarith
Copy link

commented Dec 13, 2017

No description provided.

@batrick batrick added the mgr label Dec 13, 2017

@yaarith yaarith force-pushed the yaarith:wip-mgr-module-hello branch from 1d5ede0 to 19ce7f5 Dec 14, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2017

what is the purpose of the hello_world module? is it supposed to be a sample for writing a plugin for mgr?

@yaarith

This comment has been minimized.

Copy link
Author

commented Dec 15, 2017

Indeed, a sample skeleton for a mgr module. Still need to add some examples of how to print to stdout and how to use command args.

@liewegas

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

Yeah

@yaarith yaarith force-pushed the yaarith:wip-mgr-module-hello branch from 19ce7f5 to d0d4b5a Dec 17, 2017

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

Hmm, not sure if this needs to be in with the "real" modules if it's purely for documentation purposes? We do already have the selftest module for exercising functionality in tests.

@liewegas

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

Here is nice because it is less likely to go stale, but we can move it into the docs, too. Either way!

@yaarith yaarith force-pushed the yaarith:wip-mgr-module-hello branch from d0d4b5a to edcf8ed Dec 18, 2017

@LenzGr

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

I personally would prefer it to be part of the "real" modules, as this makes it easier to find and ensures it does not go stale (like @liewegas already mentioned). Having such a skeleton is a great way of getting started with a new module.

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2017

I'm not deeply committed either way -- however, if we do put it in the main modules path, then let's have some reference to it from the documentation (and possibly a reference from this module's docstring to the documentation so that people know there's more?). If we want to avoid this module going stale then there should also be a test in qa/tasks/mgr/test_module_selftest that enables this module and then checks that no health warnings are raised.

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2018

@yaarith are you planning to come back to this?

@yaarith

This comment has been minimized.

Copy link
Author

commented Jan 16, 2018

yes, will be done hopefully today.

@yaarith yaarith force-pushed the yaarith:wip-mgr-module-hello branch from edcf8ed to 85b1a9b Jan 16, 2018

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

Oops, I didn't notice this had been updated (it still had DNM in the title).

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

retest this please (jenkins)

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

Targeting for mimic, will add another commit that excludes hello.py from packages after this

@jcsp jcsp added this to the mimic milestone Apr 23, 2018

@jcsp jcsp changed the title DNM: pybind/mgr: new 'hello world' mgr module skeleton pybind/mgr: new 'hello world' mgr module skeleton Apr 23, 2018

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

docs build failing with (transient?) failure to fetch from git

retest this please (jenkins)

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

@yaarith before we merge this, please can you add a link to your hello.rst into doc/mgr/index.rst along with the other modules, so that the docs get built.

@@ -0,0 +1 @@
from module import * # NOQA

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 24, 2018

Contributor

@yaarith might want to change this to

from .module import Module

as it's compatible with py3. and is more explicit in the sense that all we need is just the Module class.

This comment has been minimized.

Copy link
@yaarith

yaarith Apr 25, 2018

Author

Changed, thanks!


from mgr_module import MgrModule

class Module(MgrModule):

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 24, 2018

Contributor

nit, could rename this class to Hello to emphasis the fact that any class inheriting from MgrModule will be considered as a mgr plugin.

This comment has been minimized.

Copy link
@yaarith

yaarith Apr 25, 2018

Author

I tried to rename it to 'Hello', but it did not work; from out/mgr.x.log:
2018-04-25 10:05:11.963 7fdd3dd64700 -1 mgr load_subclass_of Module not found: 'hello'
2018-04-25 10:05:11.963 7fdd3dd64700 -1 mgr load_subclass_of Traceback (most recent call last):
File "/home/yaarit/ceph/ceph/src/pybind/mgr/hello/init.py", line 1, in
from .module import Module
ImportError: cannot import name Module

2018-04-25 10:05:11.963 7fdd3dd64700 -1 mgr load Class not found in module 'hello'
2018-04-25 10:05:11.963 7fdd3dd64700 -1 mgr init Error loading module 'hello': (2) No such file or directory

What am I missing?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 25, 2018

Contributor

ahh, you need change

from .module import Module

to

from .module import Hello

because what we have is but the Hello class after renaming Module to Hello.

This comment has been minimized.

Copy link
@yaarith

yaarith Apr 25, 2018

Author

Done, thanks :-)

@yaarith yaarith force-pushed the yaarith:wip-mgr-module-hello branch from 85b1a9b to a3b6ffc Apr 25, 2018

from mgr_module import MgrModule


# TODO check why class Hello does not work

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 25, 2018

Contributor

will do.

Yaarit Hatuka
pybind/mgr: add 'hello world' mgr module skeleton
This simple 'hello' mgr module prints 'hello world' to stdout and to out/mgr.x.log; for documentation purposes. Also added /doc/mgr/hello.rst.

Signed-off-by: Yaarit Hatuka <yaarithatuka@gmail.com>

@yaarith yaarith force-pushed the yaarith:wip-mgr-module-hello branch from a3b6ffc to b19e11a Apr 25, 2018

@yaarith

This comment has been minimized.

Copy link
Author

commented Apr 25, 2018

@jcsp I updated doc/mgr/index.rst with hello module, following the format of the other modules on the list; but I'm not sure that's what you meant... (didn't see any other .rst links in doc/mgr/index.rst)
Please let me know if I need to change this. Thanks!

@tchaikov tchaikov merged commit 4d942b5 into ceph:master Apr 26, 2018

4 of 5 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.