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
new backend system: events service #22344
new backend system: events service #22344
Conversation
Changed Packages
|
Uffizzi Cluster |
6beb1f8
to
21cca84
Compare
fad74bc
to
071915e
Compare
4a211f3
to
dc41b08
Compare
dc41b08
to
ef78082
Compare
ef78082
to
31fc74c
Compare
The "Microsite" check is failing with out of memory. I tried retriggering it a couple of times already. |
31b307a
to
4c01b21
Compare
@freben I tried to make use of version-bridge. However, it is not permitted to use it at such a package. Maybe, that wasn't really what you meant, though.
|
I will remove |
8c91817
to
043add7
Compare
import { Duration } from 'luxon'; | ||
import { catalogModuleBitbucketCloudEntityProvider } from './catalogModuleBitbucketCloudEntityProvider'; | ||
import { BitbucketCloudEntityProvider } from '../providers/BitbucketCloudEntityProvider'; | ||
|
||
describe('catalogModuleBitbucketCloudEntityProvider', () => { | ||
it('should register provider at the catalog extension point', async () => { | ||
const events = new TestEventsService(); | ||
const eventsServiceFactory = createServiceFactory({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test utils package could export a mockEventsServiceFactory
in the first place as well, or (structured similarly to mockServices
) a mockEventsService.factory()
, mockEventsService.service
etc or so. No need to look into that at this point though, that can be deferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't take care of that one.
Currently, the helper functions are not exported at backstage-test-utils. Rebuilding the same seems not right either.
- I could put it into backstage-test-utils. However, that would add the dependency to events-node to it.
- I could keep them separate and (a) copy or (b) export the helpers to achieve a similar setup.
- I could just add a factory implementation similar to how it was done at this test to make it simpler.
- ...
What do you think?
plugins/events-node/src/service.ts
Outdated
*/ | ||
export const eventsServiceRef = createServiceRef<EventsService>({ | ||
id: 'events.service', | ||
scope: 'root', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were looking into merging this for the mainline release, but I'm sorry we think there's one thing that still should be settled.
We were wondering if we could
- make this one plugin scoped instead
- make it depend on the plugin metadata service and prefixing subscription IDs with
<pluginId>.
Then we could possibly introduce a rootEventsServiceRef
as well in the future but only if strictly needed (and reimplement the plugin scoped one in terms of the root scoped one). This also makes the naming pattern of this one more aligned.
Does that make sense? We usually add the root ones only if necessary since they kind of by definition are less "powerful". Do you have any particular reason to have a root scoped events service already at this point?
Happy to hear your feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freben The events service is supposed to create an event-based communication across plugin boundaries. Hence, I fail to see how it can be plugin-scoped.
If just the instance itself is plugin-scoped the actual underlying communication is still root-scoped/global, it could work. Even though the benefits might be limited (e.g., the prefixing of subscription IDs).
I guess the version-bridge
was an idea to create this shared state/context, despite being plugin-scoped. However, this package is limited to web-library
roles.
We could have both, root-scoped and plugin-scoped, and the root-scoped service is used within the plugin-scoped implementations. Not sure if this would help with what you intend to achieve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you create the shared state with createRootContext
if you need one.
This choice isn't really based on what communication pattern it implements under the hood. It's based on the potential capabilities and whether it might want to contextually be aware of which plugin is using it. And you often do in the end; imagine wanting to emit plugin-tagged metrics based on their events activity for just one example among many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer, forgot about that one. I will have a look at how I can change the setup further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to plugin-scope. Please let me know if this version works for you.
043add7
to
155f921
Compare
I will take care of the merge conflicts |
Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
…r` to `EventsService` Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
…gin` to use `EventsService` Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
…ervice` Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
…d module to use `EventsService` Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
…end system Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
… migrates to `EventsService` - Fixes the support for the new backend system that was broken entirely (with and without events support). - Migrates the `BitbucketCloudEntityProvider` to use the `EventsService`. Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
155f921
to
9e527c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, I think it's time to! 🎉
Awesome work @pjungermann 👏
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
✔️ Checklist
Signed-off-by
line in the message. (more info)