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 RMAU limits and bridge blocking #612

Merged
merged 10 commits into from
Sep 30, 2021
Merged

Conversation

tadzik
Copy link
Contributor

@tadzik tadzik commented Sep 3, 2021

Sister PR with matrix-org/matrix-appservice-irc#1472, based on matrix-org/matrix-appservice-bridge#350.

Drafting since I'm planning to move some code duplicated between -slack and -irc into bridge to reduce duplication.

@tadzik tadzik requested a review from a team September 3, 2021 14:52
@@ -63,6 +65,14 @@ export class NedbDatastore implements Datastore {
private readonly teamStore: NedbDb) {
}

storeUserActivity(matrixId: string, activity: UserActivity): Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to give up on these completely since NeDB is already deprecated since a few versions ago.

@tadzik tadzik marked this pull request as ready for review September 6, 2021 14:18
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Looks good, a few concerns

@@ -7,6 +7,9 @@ homeserver:

username_prefix: "slack_"


RMAU_limit: 100
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to go lowercase with these. I'm also wondering if we should have co-located it with the room limits, but it's probably a bit late for that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I was keeping it consistent with something, but apparently it was the exact opposite. Fixed in 121e102

src/Main.ts Outdated
@@ -186,6 +214,10 @@ export class Main {
controller: {
onEvent: async(request) => {
const ev = request.getData();
if (this.bridgeBlocker?.isBlocked) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might still want to handle admin commands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excluded in 27aa15a

src/Main.ts Outdated
}

async disableRtm() {
await this.slackRtm?.disconnectAll().then(() => log.info("Disabled RTM"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooi, why the then? This should be identical?

Suggested change
await this.slackRtm?.disconnectAll().then(() => log.info("Disabled RTM"));
await this.slackRtm?.disconnectAll();
log.info("Disabled RTM");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The alternative would fire the log.info() even if slackRtm is undefined and thus not really disabled. It does look confusing though, an if may be less concise but is probably more obvious. Will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in bc8a22e

}

async blockBridge() {
await this.slackBridge.disableHookHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would add logs here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f085cff

}

async unblockBridge() {
if (this.slackBridge.config.rtm?.enable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f085cff

@tadzik tadzik merged commit e07f9dd into develop Sep 30, 2021
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.

2 participants