Make prefix configurable

matrix-hacks / matrix-puppet-slack

puppet style slack bridge for matrix

47 stars 17 forks source link

Make prefix configurable #85

Closed jeffcasavant closed 5 years ago

jeffcasavant commented 5 years ago

This moves the prefix into the config file. I'm currently running this code with no ill effects, it's still able to forward messages and create rooms & ghost users with no problem.

Fixes #34

thomas-profitt commented 5 years ago

I'm really glad to finally see this!

There's two problems left to solve:

We may need a way to make it clear that once the bridge has been running, creating rooms and users, its prefix can't be changed. A comment in config.json would do, if JSON supported comments.
If a matrix-puppet-slack user upgrades to a version with this merged, they'd need to edit their existing config to get their bridge to work again. We need a way to default to the current "slack" for users who don't have a prefix configured (e.g., check if it exists in config and write it if not)

jeffcasavant commented 5 years ago

@thomas-profitt thoughts? I've tested config regen with & without a defined prefix, and it works great. I have not tested actual bridge running without a prefix defined, but since it uses the same method to set the value, it should be fine.

I added a note to the readme on initial configuration indicating that it can't be changed. I'm inclined to think there's probably a way to detect that the prefix has been changed & warn the user, but I'm not sure how the bridge keeps track of what rooms/ghosts it knows about.

Thinking about it more - I think the best way to approach this would be to get an implementation for extend() and load both configs, overwriting the default config with the local config.

thomas-profitt commented 5 years ago

The only concern I have is the need to check config.prefix === undefined ? 'slack' : config.prefix each time config.prefix needs to be fetched; I can see future developers, who have their config set, just using config.prefix, and it working fine for them, but actually creating a fairly nasty state disenfragmentortion for users who don't have config.prefix set.

We could add something to startup to rewrite the config file to set config.prefix for users who don't have it, and then later in execution safely assume it's not null.

We could also abstract config so it's an object that's got defaults to extend the state in the JSON.

jeffcasavant commented 5 years ago

I'll hop back on this this week sometime.

jeffcasavant commented 5 years ago

@thomas-profitt sorry it took me so long to get back to this.

I went the migration-in-place route. If the process is started with a config file that's missing the prefix key, it sets it to 'slack' and writes config.json (neatly indented with 4 spaces). It then terminates the process and requests the user start it again.

If it has any issues writing the file, it has already logged the key and value it was going to set, so it logs that error and crashes expecting the user to manually edit config.json to include the new values.

thomas-profitt commented 5 years ago

Good thinking!

Thanks for coming back to this :+1: