Remove rollbar-backend legacy backend
#4778
Conversation
The rollbar backend plugin did not have the id expected by the frontend plugin and all rollbar to backend requests failed. Signed-off-by: James Andrew Vaughn <[email protected]>
Changed Packages
|
modethirteen
force-pushed
the
rollbar-backend-remove-legacy-backend
branch
3 times, most recently
from
9ae6511 to
8f488c6
Compare
There was a problem hiding this comment.
Thanks @modethirteen, left a pair of comments, these some of this that I don't think we can simply change as they weren't part of the deprecation notice.
@andrewthauer, would like your feedback on this, would it be safe to make these changes I've commented on?
There was a problem hiding this comment.
I don't think we can make these changes, they weren't part of the deprecation, for those using this it would be very much an unexpected change.
There was a problem hiding this comment.
I understood the path to be total encapsulation in the new backend system plugin registration mechanism, so that might be a misalignment on my part.
It was originally the desire to keep compatibility with the existing RollbarApi constructor that created all the branching code we wanted to avoid in #4776.
If we need to keep RollbarApi as a public interface, is it ok if the constructor now depends on CacheService? This is a major version bump anyways so folks could use the RollbarApi directly but update their call sites to include CacheService. That feels like a reasonable compromise.
There was a problem hiding this comment.
Yeah, you're moving a bit too fast for me and now these PRs have been changed from what I had seen so it's getting really hard for me to work back on this. Sorry, wasn't expecting you to make changes and work more linearly.
There was a problem hiding this comment.
No problem, it's not exactly working hours in North America. I removed the more controversial public interface changes (restored the api module export from index.ts) and I'll await feedback on how to proceed with #4776, which breaks constructor compatibility. I'm a little unsure of how to proceed with a dependency change if we're still exposing the RollbarApi constructor after this refactor - I have one possible path outlined in #4776 (comment).
BREAKING CHANGE: This removes the deprecated createRouter interface. Going forward the rollbar backend plugin must be registered using createBackendPlugin Signed-off-by: James Andrew Vaughn <[email protected]>
modethirteen
force-pushed
the
rollbar-backend-remove-legacy-backend
branch
from
8f488c6 to
91da254
Compare
There was a problem hiding this comment.
Thanks, I think this is pretty close to being able to approve and merge. Left one comment about the changeset, easy fix. Then to get the CI green you'll need to update the API Report and commit the changes, details on that here: https://github.com/backstage/community-plugins/blob/main/CONTRIBUTING.md#api-reports
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@backstage-community/plugin-rollbar-backend': major | |||
There was a problem hiding this comment.
| '@backstage-community/plugin-rollbar-backend': major | |
| '@backstage-community/plugin-rollbar-backend': minor |
Missed this last time around, as this plugin/package is still 0.x we use minor for breaking changes following the upstream Backstage conventions: https://github.com/backstage/backstage/blob/master/REVIEWING.md#reviewing-changeset-bump-levels
Hey, I just made a Pull Request!
Per #4776 (review), this is a breaking change to remove
rollbar-backend's deprecated legacy backend interface, enabling a cleaner approach to upcoming improvements.✔️ Checklist
Signed-off-byline in the message. (more info)