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.

Sign up for GitHub

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

Jump to bottom

Since 4376679b997bc92d613974d4daa2091e696c8ee5 install instructions require setting the NODE_ENV or editing default.yaml #365

Open
deepbluev7 opened this issue Aug 30, 2022 · 7 comments
Open

Since 4376679b997bc92d613974d4daa2091e696c8ee5 install instructions require setting the NODE_ENV or editing default.yaml #365

deepbluev7 opened this issue Aug 30, 2022 · 7 comments
Assignees
@Yoric

Comments

@deepbluev7
Copy link
Contributor

Describe the bug
Since #347 mjolnir doesn't start anymore. I fixed that by explicitly setting the node env to development:

neko:lib/synapse/mjolnir remotes/origin/gnuxie/audit-yarn-lock~1 (bisect) # node lib/index.js
Tue, 30 Aug 2022 16:41:06 GMT [INFO] [index] Starting bot...
Tue, 30 Aug 2022 16:41:06 GMT [ERROR] [MatrixHttpClient (REQ-1)] [Error: Error during MatrixClient request GET /_matrix/client/r0/joined_rooms: 401 Unauthorized -- {"errcode":"M_UNKNOWN_TOKEN","error":"Invalid access token passed.","soft_logout":false}]
Failed to setup mjolnir from the config /data/storage: Error: Error during MatrixClient request GET /_matrix/client/r0/joined_rooms: 401 Unauthorized -- {"errcode":"M_UNKNOWN_TOKEN","error":"Invalid access token passed.","soft_logout":false}
node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: Error during MatrixClient request GET /_matrix/client/r0/joined_rooms: 401 Unauthorized -- {"errcode":"M_UNKNOWN_TOKEN","error":"Invalid access token passed.","soft_logout":false}]

Node.js v18.6.0                                                                                                                                                                                                                                                               neko:lib/synapse/mjolnir remotes/origin/gnuxie/audit-yarn-lock~1 (bisect) # git bisect bad
4376679b997bc92d613974d4daa2091e696c8ee5 is the first bad commit
commit 4376679b997bc92d613974d4daa2091e696c8ee5                                                                                                                                                                                                                               Author: Jess Porter <github@lolnerd.net>
Date:   Tue Aug 16 15:51:18 2022 +0100

    load config yaml manually, remove more references to static config (#347)

To Reproduce
Steps to reproduce the behavior:

  1. Follow install instructions that tell you do edit development.yaml
  2. Try to start mjolnir
  3. Startup fails

Expected behavior
Mjolnir starts

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: Gentoo
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.
@Gnuxie
Copy link
Contributor

Gnuxie commented Sep 5, 2022

For context: these are the instructions being referred to

@Yoric Yoric mentioned this issue Sep 6, 2022
Open
@Yoric Yoric self-assigned this Sep 6, 2022
@Yoric
Copy link
Contributor

Yoric commented Sep 6, 2022 •
edited

Investigating. @deepbluev7, are you using pantalaimon? Just to be certain, did you specify ACCESS_TOKEN in your config file?

I don't think it's the cause of your issue, but might as well get rid of the simple hypothesis first :)

@Yoric
Copy link
Contributor

Yoric commented Sep 6, 2022

@Gnuxie @jesopo So, to confirm:

We should fix one of these :)

Which one is the expected behavior?

@deepbluev7
Copy link
Contributor Author

Investigating. @deepbluev7, are you using pantalaimon? Just to be certain, did you specify ACCESS_TOKEN in your config file?

I am not using that and the issue is simply that the mentioned commit uses a different config file by default without setting environment variables then it did before. The fix is to either rename development.yaml to default.yaml or set NODE_ENV=development :3

@deepbluev7
Copy link
Contributor Author

deepbluev7 commented Sep 6, 2022 •
edited

Which one is the expected behavior?

Well, arguably the old behaviour was to use development.yaml, so I would suggest to keep that, but in the end it doesn't really matter, as long as there is correct documentation and if something changes, instructions how to migrate.

@jesopo
Copy link
Contributor

jesopo commented Sep 6, 2022

@Gnuxie @jesopo So, to confirm:

* [the instructions](https://github.com/matrix-org/mjolnir/blob/main/docs/setup_selfbuild.md) claim that the user should edit `development.yaml`;

* [the code](https://github.com/matrix-org/mjolnir/pull/347/files#diff-c3095d5010e65c52737a98a5d618ea24049ebe90c8470752426081d70ed6e012) will load `default.yaml`.

We should fix one of these :)

Which one is the expected behavior?

@Yoric I think we should be telling users to use default.yaml if they're not running mjolnir for development purposes

@Yoric
Copy link
Contributor

Yoric commented Sep 7, 2022

Looks to me like the documentation has been wrong since at least bf7f131 . I'll fix that.

Yoric added a commit that referenced this issue Sep 7, 2022
@Yoric
Tweaking config instructions and initialization - resolves #365
2e78773 
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 added a commit that referenced this issue Sep 7, 2022
@Yoric
Tweaking config instructions and initialization - resolves #365
fdccffd 
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.
Gnuxie added a commit that referenced this issue Nov 22, 2022
@Gnuxie
Use the npm package config to load the config.
eeaa9d3 
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.
@Gnuxie Gnuxie mentioned this issue Nov 22, 2022
Merged
Gnuxie added a commit that referenced this issue Nov 23, 2022
@Gnuxie
Config fixes (#432)
333c55e 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Yoric Yoric

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@Yoric @jesopo @deepbluev7 @Gnuxie