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

Adds config reload handler #222

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

amimof
Copy link

@amimof amimof commented Sep 8, 2021

Adds a new handler that accepts POST /reload which reloads the teams configuration file. This is very useful when running prometheus-msteams with https://github.com/jimmidyson/configmap-reload to reload configuration when changes occurs in a ConfigMap.

@amimof
Copy link
Author

amimof commented Nov 4, 2021

@Knappek What do you think about this enhancement. Can you please have a look and give me your input. Thanks!

@Knappek
Copy link
Collaborator

Knappek commented Nov 6, 2021

@amimof sorry for the late reply. I like the idea! Unfortunately it does not yet work as expected as you are only returning the content of the config file. You have to reset the service to actually use the new config file.
Could you please fix that and test it on your end whether it works. Would also be great to have a unit test for that (if that makes sense?). In the end, please also add a short documentation to the README.
Thanks!

@amimof
Copy link
Author

amimof commented Nov 11, 2021

Right, I'll make sure to have somthing working as soon as possible. Thanks!

@amimof
Copy link
Author

amimof commented Nov 26, 2021

@Knappek I've managed to get the reload handler to reload the services. Also added a test that verifies it. Have a look and tell me what you think.

cmd/server/main.go Outdated Show resolved Hide resolved
cmd/server/main_test.go Show resolved Hide resolved
@Knappek
Copy link
Collaborator

Knappek commented Dec 2, 2021

@amimof looks really good already, just some minor comments. Awesome new feature 👍

@amimof amimof requested a review from Knappek December 3, 2021 09:39
Return 500 if config reload fails
* Added function `setupServices()` which is called on app startup and when `POST /reload` is called
* Services are now correctly reloaded
* Added `ReloadRoutes` in transport package which adds new routes on an exiting echo server
@amimof
Copy link
Author

amimof commented Apr 14, 2022

@Knappek Please review

@hokagegano
Copy link

Hi,

Any news for this feature ?
It seems to be a good idea to have this capabilty.

You need some help for that ?

Regards
Arnaud DALLIES

@amimof
Copy link
Author

amimof commented Jan 3, 2023

@hokagegano Just needs approval from @Knappek

@Knappek
Copy link
Collaborator

Knappek commented Jan 3, 2023

As of #254 I'm not actively contributing anymore. Please reach out to owners of this project. Thank you!

@amimof
Copy link
Author

amimof commented Jan 5, 2023

As of #254 I'm not actively contributing anymore. Please reach out to owners of this project. Thank you!

My bad, who is the owner? @bzon?

func ReloadRoutes(e *echo.Echo, routes []Route, logger log.Logger) {
for _, r := range routes {
level.Debug(logger).Log("request_path_added", r.RequestPath)
addRoute(e, r.RequestPath, r.Service, logger)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amimof

Thanks for your great work. Overall, it looks good to me.
I have some concerns.

If we use addRoute here, does it mean all obsolete routes would remain?

I am not sure is there elegant way to reload the routes at once. At the worst case, we can list routes, and then delete them except /metrics, /debug/pprof/*, /config and /reload. It would be great if we can add test a case as well.

Let me know if you have any concern.

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

4 participants