-
Notifications
You must be signed in to change notification settings - Fork 56
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
Tweaking config instructions and initialization - resolves #365 #369
base: main
Are you sure you want to change the base?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.yaml
if available, without additional instructions needed;default.yaml
, which is read without additional instructions;development.yaml
;