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

Hook return #526

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

Conversation

Projects
None yet
4 participants
@Josue-T
Copy link
Contributor

Josue-T commented Aug 29, 2018

The problem

  • There are no posibility to return a value from a hook.

Solution

Add a possibility to return a value by creating a Json file in the $YNH_STDRETURN path.

To return a value the hook should look like something like that :

echo '["small test"]' > $YNH_STDRETURN

PR Status

Tested for #517 but need some other test because the value returned by hook_callback has been changed.

How to test

  • Checkout the branch
  • Test the #517 witch need this feature.

Validation

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

@Josue-T Josue-T referenced this pull request Aug 29, 2018

Open

[enh] Hooks for custom DNS confs #517

0 of 4 tasks complete

@Josue-T Josue-T requested a review from alexAubin Aug 30, 2018

@alexAubin
Copy link
Member

alexAubin left a comment

I still need to test it (no time for that right now) but overall that looks good ! Thanks for implementing the changes we discussed at Brique Camp 👍 😋

Show resolved Hide resolved src/yunohost/hook.py Outdated
Show resolved Hide resolved src/yunohost/hook.py Outdated
@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Sep 3, 2018

There are conflict to resolve :/

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Sep 3, 2018

There are conflict to resolve :/

Should be fixed

Show resolved Hide resolved src/yunohost/hook.py Outdated

@Josue-T Josue-T removed the work needed label Dec 4, 2018

except KeyError:
result[state][name] = [path]

result[name] = {'path' : path, 'state' : state, 'stdreturn' : hook_return }

This comment has been minimized.

@zamentur

zamentur Jan 4, 2019

Contributor

This PR doesn't change a lot of code using the 'state' key, like here

for part in ret['succeed'].keys():

The easiest is to keep the result[state][name] like that so something like:

result[state][name] = {'path' : path, 'state' : state, 'stdreturn' : hook_return }

This comment has been minimized.

@Josue-T

Josue-T Jan 4, 2019

Contributor

I think I changed that because it was a request from @alexAubin . But not sure.

Can @alexAubin confirm this ?

This comment has been minimized.

@alexAubin

alexAubin Jan 4, 2019

Member

Uh yeah I don't understand the trouble here ?

To me it doesn't make sense to just put things in either the "succeed" or "failed" box instead of just having directly a dictionnary of hook name (result[name]) ?

Dunno why that troublesome to do this instead of keeping the existing result[state][name] ?

This comment has been minimized.

@zamentur

zamentur Jan 4, 2019

Contributor

To do this, you need to change a lot of things and you can't just make:

ret = hook_callback(...)
if ret['failed']:
    # Some error management

This kind of pattern is used in a lot of piece of code. We can't change the format of the return without check all hook_callback call...

This comment has been minimized.

@zamentur

zamentur Jan 4, 2019

Contributor

In more, previously there was possibility to get several hook path for the same name. I think it could be several path on setup using custom hook. I believe mailman_ynh use this feature.

This comment has been minimized.

@Josue-T

Josue-T Jan 6, 2019

Contributor

Well,

So what should I do for that now ?

@YunoHost YunoHost deleted a comment from zamentur Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment