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

Implement separate bot users per service #573

Merged
merged 54 commits into from
Jan 13, 2023
Merged

Conversation

justinbot
Copy link
Contributor

@justinbot justinbot commented Nov 17, 2022

Introduces the ability for Hookshot to present itself as multiple different bots, each of which can handle one or more specific services. This is a pretty fundamental change, and as a result touches almost everywhere.

It should be relatively reviewable by commit, but will require significant testing of different scenarios to make sure things work correctly.

Implementation

Fundamentally, this is breaking an assumption throughout the codebase that there is one global bot. The implications are:

  • Before interacting with a room, we need to find a bot which is in the room.
    • But also, multiple bots could be in the same room.
  • When doing so in the context of a particular service (e.g. sending a feed update), we need to prioritize the service bot over the default bot.

Connections

However, this does not mean connections are tied to a specific bot user.
We need to choose the right bot for a connection, but this is done at runtime based on which bots are in the room (again prioritizing service bots).

Fixes #436

@justinbot justinbot self-assigned this Nov 17, 2022
@justinbot justinbot requested a review from a team as a code owner November 17, 2022 21:27
src/Bridge.ts Outdated Show resolved Hide resolved
src/Bridge.ts Outdated Show resolved Hide resolved
src/Bridge.ts Outdated Show resolved Hide resolved
@justinbot justinbot marked this pull request as draft November 17, 2022 21:47
src/Bridge.ts Outdated Show resolved Hide resolved
@justinbot justinbot force-pushed the justinbot/service-bots-again branch 2 times, most recently from fd7bee2 to c65d67f Compare November 29, 2022 19:09
src/Bridge.ts Show resolved Hide resolved
src/Bridge.ts Outdated Show resolved Hide resolved
src/Bridge.ts Outdated Show resolved Hide resolved
src/Bridge.ts Show resolved Hide resolved
@justinbot justinbot marked this pull request as ready for review December 2, 2022 19:07
src/Bridge.ts Show resolved Hide resolved
src/Bridge.ts Show resolved Hide resolved
src/Connections/GenericHook.ts Show resolved Hide resolved
src/Connections/GenericHook.ts Show resolved Hide resolved
src/Managers/BotUsersManager.ts Outdated Show resolved Hide resolved
@justinbot
Copy link
Contributor Author

@Half-Shot (I can't reply to this comment directly for some reason)

This should probably be a dedicated handover function. Our connections are slightly stateful and it's better to not recreate them v.s. updating them, if we can avoid it.

It doesn't seem like it can be avoided, since "handing over" a connection really means instantiating it with a different intent in the opts.
If there are specific statefulness concerns I would say let's try and address those, since anyway connections need to be recreated from state events when e.g. Hookshot is restarted.

Copy link
Contributor

@tadzik tadzik left a comment

Choose a reason for hiding this comment

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

I'm happy with this – I'm still curious about #573 (comment) though (defering to Half-Shot).

@Half-Shot Half-Shot enabled auto-merge (squash) January 13, 2023 15:31
@Half-Shot Half-Shot merged commit 9a7839c into main Jan 13, 2023
@Half-Shot Half-Shot deleted the justinbot/service-bots-again branch January 13, 2023 15:32
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.

Allow creation of "service bots" which can be used for specific service(s)
4 participants