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

(change) Make Notifier interface publicly implementable #214

Closed
shezadkhan137 opened this issue Feb 22, 2022 · 6 comments · Fixed by #227
Closed

(change) Make Notifier interface publicly implementable #214

shezadkhan137 opened this issue Feb 22, 2022 · 6 comments · Fixed by #227
Labels
change This is a change in the code that should not affect the users p1 High priority

Comments

@shezadkhan137
Copy link

shezadkhan137 commented Feb 22, 2022

Motivation

Make the Notifier interface publicly implementable.

Hi, thanks for making this library! I would like to output feature flag changes to our internal Grafana via the Annotations API.

The current Webhook notifier doesn't satisfy our need, as we would like to format the text for the annotation based on the diff. We could do this via an intermediate web server that translates the Webhook notifier payload for Grafana, but it would be nice to do it without the intermediate server.

Requirements

Ideally, go-feature-flag should allow the notifier interface to be implemented publically and support custom notifiers passed in completely via the Config. Currently is not possible because the GetNotifier interface returns the internal Notifier interface. Also, the Notifier interface accepts an internal model.DiffCache struct.

Would it be possible to make these part of the public API? I think it may be just a matter of taking it out of the internal package and into a public notifier package?

I'm more than willing to put a PR in, if you are okay with making it public. Thanks!

@shezadkhan137 shezadkhan137 added change This is a change in the code that should not affect the users needs-triage A priority should be added to the issue labels Feb 22, 2022
@shezadkhan137 shezadkhan137 changed the title Make Notifier interface publicly implementable (change) Make Notifier interface publicly implementable Feb 22, 2022
@urso
Copy link

urso commented Mar 4, 2022

Other interfaces like Exporter face the same issue. Maybe it would make sense to rename internal to pkg or move some more packages from internal one level up?

@thomaspoignant
Copy link
Owner

thomaspoignant commented Apr 8, 2022

@shezadkhan137 Sorry I just see your issue today.
I will check what I can do to change the way the notifier works to make them public.

And feel free to open a PR for that.

@urso thanks for the idea I will look at them.

@thomaspoignant thomaspoignant added p1 High priority and removed needs-triage A priority should be added to the issue labels Apr 8, 2022
@thomaspoignant
Copy link
Owner

Other interfaces like Exporter face the same issue. Maybe it would make sense to rename internal to pkg or move some more packages from internal one level up?

I have exposed FeatureEvent publicly for the Exporter, now it should work. See #225.

@thomaspoignant
Copy link
Owner

thomaspoignant commented Apr 8, 2022

I am really sorry for the delay @shezadkhan137, I have released a v0.20.0 version that allows creating custom notifiers.
I have added an example of how to do that in the documentation.

Feel free to give me any feedback if it works well for you.

@shezadkhan137
Copy link
Author

Wow, thanks so much @thomaspoignant. Will give it a go and let you know :-)

@urso
Copy link

urso commented Apr 11, 2022

I have exposed FeatureEvent publicly for the Exporter, now it should work. See #225.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change This is a change in the code that should not affect the users p1 High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants