Tweaking config instructions and initialization - resolves #365 #369
Conversation
Configuration instructions encourage the user to use a configuration file called `development.yaml` (for self-build) or `production.yaml` (for Docker build) but do not provide instructions to point Mjölnir to read from these files. This patch: - allows Mjölnir to read `production.yaml` if available, without additional instructions needed; - change the instructions for self-build to use `default.yaml`, which is read without additional instructions; - add instructions in case the user wishes to use `development.yaml`; - tweak the error message in case the config file isn't setup at all to clarify this for users.
Yoric
force-pushed
the
yoric/issue-365
branch
from
2e78773
to
fdccffd
Compare
|
The reason this issue exists is because we removed
I also don't know if this is a good thing, it makes it easier to just ignore the config entirely without checking if the defaults are correct for the setup you want. |
the library forced configs to be static, so we did need to remove it |
| throw new Error(`Invalid access token in configuration file ${config_path}. ` | ||
| + "This usually indicates that Mjölnir's configuration file was not setup " | ||
| + "or that Mjölnir is using the wrong configuration file."); |
There was a problem hiding this comment.
This does not work if they are using pantalaimon. It will say this every time.
It also is confusing because yes the token is invalid but that's not because of something the user has done, it's because they haven't done something (made a config available) or we have silently loaded a default config instead of exploding.
| @@ -22,7 +22,7 @@ import { read as configRead } from "../../src/config"; | |||
| import { RULE_ROOM, RULE_SERVER, RULE_USER } from "../../src/models/ListRule"; | |||
|
|
|||
| function createTestMjolnir(defaultShortcode: string|null = null): Mjolnir { | |||
| const config = configRead(); | |||
| const config = configRead(false); | |||
There was a problem hiding this comment.
I'm still not confident in this, since this is really trying to be an option whether to load a default if the "original" wasn't found. But we would need to change this in a few more places now that the appservice exists.
https://github.com/node-config/node-config/wiki/Using-Config-Utilities#clonedeepcopyfrom-depth |
This is what was used prior to #347. It was a nice idea motivated to drop a dependency that was confusing. It was just never followed through and was underestimated how much disruption it would cause. It was also believed that the library would mean there could only ever be one global copy of the config, It was followed up by: #369 #357 #429 https://github.com/matrix-org/mjolnir/pull/397/files #365 For simplicity sake I am reinstating the library. The practice of loading default.yaml by default is also dangerous and has led to issues multiple times in #mjolnir:matrix.org. It is a sample and not a default. In a following commit I will be adding the ability to specify the config to use from the cli.
* Use the npm package `config` to load the config. This is what was used prior to #347. It was a nice idea motivated to drop a dependency that was confusing. It was just never followed through and was underestimated how much disruption it would cause. It was also believed that the library would mean there could only ever be one global copy of the config, It was followed up by: #369 #357 #429 https://github.com/matrix-org/mjolnir/pull/397/files #365 For simplicity sake I am reinstating the library. The practice of loading default.yaml by default is also dangerous and has led to issues multiple times in #mjolnir:matrix.org. It is a sample and not a default. In a following commit I will be adding the ability to specify the config to use from the cli. * Allow config to be specified with an explicit cli argument. * Update doc to transition away from old config handling
Configuration instructions encourage the user to use a configuration file called
development.yaml(for self-build) orproduction.yaml(for Docker build) but do not provide instructions to point Mjölnir to read from these files.This patch:
production.yamlif available, without additional instructions needed;default.yaml, which is read without additional instructions;development.yaml;