-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add proposal for Provider Hook #22
Conversation
Signed-off-by: ssen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating this proposal. I think provider hooks would add additional flexibility for provider developers without introducing new concepts.
My only concern is hook ordering. I've elaborated more on this in the proposal itself.
Signed-off-by: ssen <[email protected]>
Just to clarify: is there any reason that we couldn't use our existing hooks abstraction for this? and just attach them at the provider level as specified. My main concern is adding new abstractions... if provider hooks use the same hook abstraction as our existing hooks, I'm more comfortable with this proposal. I also feel that provider hooks should be "innermost" hooks - they should run last in before, after the application author has run their hooks, and first in error, after, and finally. |
Modified the after hook order based on comments Signed-off-by: ssen <[email protected]>
Can you add any clarification on this? To be clear, I'm just making sure that a provider hook would use the same hook interface that already exists... it seems like that's the case. |
I'd also like to point out I think this makes the |
Yes, there is no change proposed in the current hook interface. The Provider Hook implementation must use the same interface that is currently defined. But the application developer should not have to register/add the hook. During running the hooks, the SDK should query the Provider implementation for any provider hooks and if it is provided run the Provider hook in the order that the standard mandates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the above discussion, I would support a change to the spec outlining this.
Interested in getting more feedback from others though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
100% agree, keeping both will be confusing. |
I really like that this gets rid of context transformers. Thanks, @s-sen!! |
@s-sen I'm going to merge this if you agree. |
@s-sen, it looks like we have consensus. Please update the OFEP status to Once the OFEP has been merged, feel free to open a PR in the spec repo with the required changes and reference this PR. Thanks for driving this spec enhancement. |
Signed-off-by: Michael Beemer <[email protected]>
Sorry - I could not do this earlier. I do not need to do any additional changes in the document I assume? The next step is to open a PR in the spec repo? |
@s-sen, nothing else is required here. The next step would be to open a PR in the spec repo and reference this PR. Thanks for driving this topic. I think it's a great enhancement. |
Add proposal for Provider Hook
Signed-off-by: ssen [email protected]