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

Remove rollbar module #124

Merged
merged 3 commits into from
May 15, 2018
Merged

Conversation

andrehjr
Copy link
Contributor

Rollbar module is currently broken when used with recent versions of the Rollbar gem. Monkeypatching the Rollbar seems like a bad practice as we can configure custom fingerprinting on Rollbar directly now. More details on #122 .

This PR:

  • Removes rollbar module
  • Adds instructions to configure Custom Fingerprinting directly on Rollbar.

Another approach would be to deprecate the module first, and then remove it. As this feature was optional and experimental, removing it seems better. Thoughts?

@wuputah
Copy link
Collaborator

wuputah commented May 13, 2018

It will break the require for anyone who was using it, which will cause the app to fail to boot. Could leave the file in there and have it echo an error message instead, but I'm not particularly bothered either way. As you said, it was an experimental feature. Not sure how much use it has in practice.

It probably warrants a 0.5.0 release.

@andrehjr
Copy link
Contributor Author

Added a warn message stating that the module was removed.

@wuputah
Copy link
Collaborator

wuputah commented May 15, 2018

That works for me! We can remove the file in 0.6.0, if we ever get there.

Thanks so much for the contribution! 💯

@wuputah wuputah merged commit 5a6f751 into zombocom:master May 15, 2018
@andrehjr andrehjr deleted the remove-rollbar-module branch May 15, 2018 16:43
@wuputah wuputah mentioned this pull request May 17, 2018
@jjb
Copy link
Contributor

jjb commented Jul 16, 2018

This should perhaps be mentioned in https://github.com/heroku/rack-timeout/blob/master/UPGRADING.md

@wuputah
Copy link
Collaborator

wuputah commented Jul 16, 2018

Good point, I'll add something!

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

3 participants