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

Add proposal for Provider Hook #22

Merged
merged 4 commits into from
Jul 29, 2022
Merged

Add proposal for Provider Hook #22

merged 4 commits into from
Jul 29, 2022

Conversation

s-sen
Copy link
Contributor

@s-sen s-sen commented Jul 26, 2022

Signed-off-by: ssen mailssen@gmail.com

Signed-off-by: ssen <mailssen@gmail.com>
Copy link
Member

@beeme1mr beeme1mr left a 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.

005-OFEP-provider-hook.md Outdated Show resolved Hide resolved
Signed-off-by: ssen <mailssen@gmail.com>
@toddbaert
Copy link
Member

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 <mailssen@gmail.com>
@toddbaert
Copy link
Member

@s-sen:

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.

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.

@toddbaert
Copy link
Member

toddbaert commented Jul 28, 2022

I'd also like to point out I think this makes the contextTransformer concept redundant. Since before hooks can transform context, a provider before hook is a great place to do such transformation. I think that if we add provider hooks, we should completely remove the contextTransformer concept.

cc @thomaspoignant @beeme1mr @weyert @justinabrahms

@toddbaert toddbaert requested a review from weyert July 28, 2022 12:46
@s-sen
Copy link
Contributor Author

s-sen commented Jul 28, 2022

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

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.

Copy link
Member

@toddbaert toddbaert left a 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.

Copy link
Member

@beeme1mr beeme1mr left a 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.

@thomaspoignant
Copy link
Member

'd also like to point out I think this makes the contextTransformer concept redundant. Since before hooks can transform context, a provider before hook is a great place to do such transformation. I think that if we add provider hooks, we should completely remove the contextTransformer concept.

100% agree, keeping both will be confusing.

@justinabrahms
Copy link
Member

I really like that this gets rid of context transformers. Thanks, @s-sen!!

@toddbaert
Copy link
Member

@s-sen I'm going to merge this if you agree.

@beeme1mr
Copy link
Member

@s-sen, it looks like we have consensus. Please update the OFEP status to APPROVED and I'll merge it in.

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 <beeme1mr@users.noreply.github.com>
@beeme1mr beeme1mr merged commit 8ed3c2b into open-feature:main Jul 29, 2022
@s-sen
Copy link
Contributor Author

s-sen commented Jul 29, 2022

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?

@beeme1mr
Copy link
Member

@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.

weyert pushed a commit to weyert/research that referenced this pull request Jan 31, 2023
Add proposal for Provider Hook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants