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

Python hooks reloaded #747

Open
wants to merge 4 commits into
base: stretch-unstable
from

Conversation

@alexAubin
Copy link
Member

commented Jul 6, 2019

The problem

This is a rewrite of #535 because there were too many changes ... :

Python hooks are required for the new diagnosis system modularity / extendability, and more generally if we want to implement some hooks in python for some reason. This could be the case for example of the famous app ssowatconf command which should be a classical regen-conf thing but can't be so far because it has too many python calls and json stuff to be implemented in bash.

Solution

Split the behavior of the hook_exec function depending on the nature of the hook. Python hooks are to be modules loaded by yunohost and expected to have a main function.

PR Status

Tested and working

How to test

Create some hook with a main function (e.g. just displaying hello world) and call yunohost hook list your_hook_type and yunohost diagnosis callback your_hook_type

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@alexAubin alexAubin added this to the 3.7.x milestone Jul 6, 2019

@alexAubin alexAubin referenced this pull request Jul 6, 2019
0 of 4 tasks complete
src/yunohost/hook.py Outdated Show resolved Hide resolved
@Josue-T

Josue-T approved these changes Jul 6, 2019

Copy link
Contributor

left a comment

Didn't tested, but look good to me.

@alexAubin alexAubin force-pushed the python-hooks-reloaded branch from 099827f to 8c4d136 Jul 13, 2019

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

Merging soonish

# Check the type of the hook (bash by default)
hook_type = "bash"
# (non-bash hooks shall start with something like "#!/usr/bin/env language")
hook_ext = os.path.splitext(path)[1]

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 4, 2019

Member

https://gist.github.com/LeMeteore/cac019562822388a5d11c0ead629848c#file-transform-py-L35-L37 you might want to use this kind of code for file type detection (that also handle python script without a ".py" at the end but a shebang)

sys.path = [dir_] + sys.path
module = import_module(name)

# TODO : We might want to check here that it's a tuple

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 4, 2019

Member

Aren't you actually doing the check just bellow '-' ?


# TODO : We might want to check here that it's a tuple
# containing an int + a dict ?
ret = module.main(args, env, loggers)

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 4, 2019

Member

Shouldn't we spawn a new python process that launch the python instead of calling it in the current python process? That would be better security and bug wise I think (but idk how hard would it be to do that)

This comment has been minimized.

Copy link
@alexAubin

alexAubin Aug 18, 2019

Author Member

Hm I kinda chose to do this because that's supposedly less ressource expensive, considering that we're likely to import code from yunohost's in each of those (e.g. 10~15ish hooks in the context of diagnosis), each likely to do stuff like importing requests or other libs that take hell of time to load on RPi for example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.