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

[enh] Be able to define hook to trigger when changing a setting #654

Merged
merged 2 commits into from Feb 24, 2019

Conversation

Projects
None yet
2 participants
@alexAubin
Copy link
Member

commented Feb 19, 2019

The problem

c.f. #640 and other similar use case : we need to be able to trigger specific code when updating a setting value. For instance, to trigger a regenconf of nginx or ssh or whatever.

Solution

Introduce a decorator @post_change_hook("your.setting.name") to define what code to trigger after the setting is changed. A dummy example is :

@post_change_hook("example.int")
def myfunc(old_value, new_value):
    print("In hook")
    print(old_value)
    print(new_value) 

PR Status

Tested, ready for review

How to test

Uncomment the example at the end and try to run for instance yunohost settings set example.int -v 1664

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
Show resolved Hide resolved src/yunohost/settings.py Outdated
Show resolved Hide resolved src/yunohost/settings.py
def post_change_hook(setting_name):
def decorator(func):
assert setting_name in DEFAULTS.keys(), "The setting %s does not exists" % setting_name
assert setting_name not in post_change_hooks, "You can only register one post change hook per setting (in particular for %s)" % setting_name

This comment has been minimized.

Copy link
@rds13

rds13 Feb 20, 2019

Contributor

I understand this cover use case for #640. But in other use case it could be useful to have a list of hook functions associated with a setting name. What do you think ?

This comment has been minimized.

Copy link
@alexAubin

alexAubin Feb 21, 2019

Author Member

Hmyea I was a bit lazy to think about that so I just added this constrain 😅

My idea is that it should be easy to implement once we need it

This comment has been minimized.

Copy link
@rds13

rds13 Feb 21, 2019

Contributor

I see you have already done it ! 😄 I cannot fix the documentation at the top of this PR but may be it's not very important.

This comment has been minimized.

Copy link
@alexAubin

alexAubin Feb 22, 2019

Author Member

Uuuuh, not sure we're talking about the same thing ?

Ain't we talking about the ability or not to have multiple @post_change_hook("name") for the same "name" ?

e.g.

@post_change_hook("foo")
def foo1(...):
   ...

@post_change_hook("foo")
def foo3(...):
   ...

?

What I implemented in c029ccb is instead feeding the setting name to the function ?

This comment has been minimized.

Copy link
@rds13

rds13 Feb 22, 2019

Contributor

Sorry I messed up. I was indeed talking first about having multiple hook functions for the same setting but it’s not mandatory.
Then I switched to talking about having the setting name as a parameter.

@alexAubin alexAubin added this to the 3.5.x milestone Feb 21, 2019

@rds13

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

It looks like this can be merged now.

@alexAubin alexAubin merged commit 0320ca9 into stretch-unstable Feb 24, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the trigger-actions-when-changing-settings branch Feb 24, 2019

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.