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

fix: remove downcase from hook_type render #62

Closed
wants to merge 1 commit into from

Conversation

fionera
Copy link

@fionera fionera commented Apr 21, 2021

This fixes #56 by removing the downcase call. Because of this call the tableize function got configreport which then got transformed to configreports. Instead it should have been config_reports.

This fixes theforeman#56 by removing the downcase call. Because of this call the tableize function got `configreport` which then got transformed to `configreports`. Instead it should have been `config_reports`.
@lzap
Copy link
Member

lzap commented Apr 22, 2021

Thank you, however I am a bit worried merging this - we will completely break all users who already have these hooks implemented :-( The only user-friendly solution would be to trigger two hooks in this case and documenting this behavior in the README too. Would you mind refactoring it that way?

Note we have completely new plugin which aims to deliver integration with Foreman using more user-friendly webhooks, you can customize payload via foreman templating and much more. It is way more capable, there is also a shell hooks smart proxy plugin which allows you to run shell scripts on those events.

We are just about to announce the first release of the plugin, the last missing bit is finalizing documentation. Expect it in few weeks, but the plugin is already in 2.4 release repositories so you can start using that right away.

@fionera
Copy link
Author

fionera commented Apr 22, 2021

Thank you, however I am a bit worried merging this - we will completely break all users who already have these hooks implemented :-( The only user-friendly solution would be to trigger two hooks in this case and documenting this behavior in the README too. Would you mind refactoring it that way?

The Issue we had and how I found this in the first place was that we noticed that our prometheus metrics start to disappear after the hook call, because of the Error. I dont know much more about why and how but by fixing this, the error disappeard. I dont know much ruby since I mostly write Go, so I dont really know how to tackle this refactor.

Thanks for the poke with the webhooks :D Will look into it, since thats exactly what our hook does rn

@lzap
Copy link
Member

lzap commented Apr 23, 2021

In that case I am going to close this one, we do not want to break existing hooks. Thank you!

@lzap lzap closed this Apr 23, 2021
@fionera fionera deleted the patch-1 branch April 23, 2021 14:29
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.

config_report hook generates log warning
2 participants