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

Check config before starting MM #3450

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

KristjanESPERANTO
Copy link
Contributor

I think it might be a good idea to check the config at every start.

@khassel
Copy link
Collaborator

khassel commented May 14, 2024

We implemented the possibility to use a config.js.template instead of a config.js (where the config.js is created while startup).

At the moment the check fails if there is no config.js:

[2024-05-14 21:51:17.549] [ERROR] File not found: /opt/magic_mirror/config/config.js

So if we want this check the code must be changed that no error is thrown if no config.js exists but a config.js.template.
If we want to check config.js.template in such a case I don't now if the check can handle Variables ${XY} which can be present in config.js.template. There is also a env variable MM_CONFIG_FILE which is used to set another config file location. I don't know if the check respects this variable (it should).

Other point: Any idea why the test fails?

@bugsounet
Copy link
Contributor

bugsounet commented May 19, 2024

Why not hard coding this check in main core of MM² before loading config ?

in this case, it's +/- related to #3445

@sdetweil
Copy link
Collaborator

in general I am in favor of not starting if there is a fatal config error .

not like now where we start with a built in dummy config, unlike whatever the user expected.

@khassel
Copy link
Collaborator

khassel commented May 19, 2024

I agree with the idea, I just wanted to point out that the current approach doesn't fit all use cases...

Why not hard coding this check in main core of MM² before loading config ?

I think loadConfig in app.js must be executed before the check because in this function the config.js is created when using config.js.template.

@KristjanESPERANTO
Copy link
Contributor Author

I'd like to take a closer look, but it looks like I will not find the time soon.

@KristjanESPERANTO
Copy link
Contributor Author

I think loadConfig in app.js must be executed before the check because in this function the config.js is created when using config.js.template.

The config check is now called from inside the loadConfig after handling the config file. So I think it should now cover all use cases:

  1. normal config.js
  2. MM_CONFIG_FILE and
  3. config.js.template

in general I am in favor of not starting if there is a fatal config error

Now if the config check fails, MM will not start:

kristjan@debian:~/MagicMirror$ npm run start

> magicmirror@2.29.0-develop start
> DISPLAY="${DISPLAY:=:0}" ./node_modules/.bin/electron js/electron.js

[2024-09-18 03:31:25.980] [LOG]   Starting MagicMirror: v2.29.0-develop 
[2024-09-18 03:31:25.999] [LOG]   Loading config ... 
[2024-09-18 03:31:26.001] [LOG]   config template file not exists, no envsubst 
[2024-09-18 03:31:26.319] [INFO]  Checking file...  /home/kristjan/MagicMirror/config/config.js 
[2024-09-18 03:31:26.343] [ERROR] Your configuration file contains syntax errors :( 
[2024-09-18 03:31:26.343] [ERROR] Line 193 column 3: Parsing error: Unexpected token { 
kristjan@debian:~/MagicMirror$ 

@khassel
Copy link
Collaborator

khassel commented Sep 18, 2024

@KristjanESPERANTO can you please solve the merge conflicts, thanks!

@khassel khassel merged commit 866419e into MagicMirrorOrg:develop Sep 18, 2024
6 checks passed
@KristjanESPERANTO KristjanESPERANTO deleted the checkconfig branch September 18, 2024 17:38
khassel pushed a commit that referenced this pull request Sep 28, 2024
…ve (#3565)

after adding check:config to the MM startup process, #3450, we
accidentally discover module positions more than once, and write the
file each time..

add a check to see if we have done this work already
@khassel khassel mentioned this pull request Sep 30, 2024
khassel added a commit that referenced this pull request Sep 30, 2024
## [2.29.0] - 2024-10-01

Thanks to: @bugsounet, @dkallen78, @jargordon, @khassel,
@KristjanESPERANTO, @MarcLandis, @rejas, @ryan-d-williams, @sdetweil,
@skpanagiotis.

> ⚠️ This release needs nodejs version `v20` or `v22`, minimum version
is `v20.9.0`

### Added

- [compliments] Added support for cron type date/time format entries mm
hh DD MM dow (minutes/hours/days/months and day of week) see
https://crontab.cronhub.io for construction (#3481)
- [core] Check config at every start of MagicMirror² (#3450)
- [core] Add spelling check (cspell): `npm run test:spelling` and handle
spelling issues (#3544)
- [core] removed `config.paths.vendor` (could not work because `vendor`
is hardcoded in `index.html`), renamed `config.paths.modules` to
`config.foreignModulesDir`, added variable `MM_CUSTOMCSS_FILE` which -
if set - overrides `config.customCss`, added variable `MM_MODULES_DIR`
which - if set - overrides `config.foreignModulesDir`, added test for
`MM_MODULES_DIR` (#3530)
- [core] elements are now removed from `index.html` when loading script
or stylesheet files fails
- [core] Added `MODULE_DOM_UPDATED` notification each time the DOM is
re-rendered via `updateDom` (#3534)
- [tests] added minimal needed node version to tests (currently v20.9.0)
to avoid releases with wrong node version info
- [tests] Added `node-libgpiod` library to electron-rebuild tests
(#3563)

### Removed

- [core] removed installer only files (#3492)
- [core] removed raspberry object from systeminformation (#3505)
- [linter] removed `eslint-plugin-import`, because it doesn't support
ESLint v9. We will reenter it later when it does.
- [tests] removed `onoff` library from electron-rebuild tests (#3563)

### Updated

- [weather] Updated `apiVersion` default from 2.5 to 3.0 (#3424)
- [core] Updated dependencies including stylistic-eslint
- [core] nail down `node-ical` version to `0.18.0` with exception
`allow-ghsas: GHSA-8hc4-vh64-cxmj` in `dep-review.yaml` (which should
removed after next `node-ical` update)
- [core] Updated SocketIO catch all to new API
- [core] Allow custom modules positions by scanning index.html for the
defined regions, instead of hard coded (PR #3518 fixes issue #3504)
- [core] Detail optimizations in `config_check.js`
- [core] Updated minimal needed node version in `package.json`
(currently v20.9.0) (#3559) and except for v21 (no security updates)
(#3561)
- [linter] Switch to ESLint v9 and flat config and replace
`eslint-plugin-unicorn` by `@eslint/js`
- [core] fix discovering module positions twice after #3450

### Fixed

- Fixed `checks` badge in README.md
- [weather] Fixed issue with the UK Met Office provider following a
change in their API paths and header info.
- [core] add check for node_helper loading for multiple instances of
same module (#3502)
- [weather] Fixed issue for respecting unit config on broadcasted
notifications
- [tests] Fixes calendar test by moving it from e2e to electron with
fixed date (#3532)
- [calendar] fixed sliceMultiDayEvents getting wrong count and
displaying incorrect entries, Europe/Berlin (#3542)
- [tests] ignore `js/positions.js` when linting (this file is created at
runtime)
- [calendar] fixed sliceMultiDayEvents showing previous day without
config enabled

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Michael Teeuw <michael@xonaymedia.nl>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ross Younger <crazyscot@gmail.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: jkriegshauser <joshuakr@nvidia.com>
Co-authored-by: illimarkangur <116028111+illimarkangur@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: vppencilsharpener <tim.pray@gmail.com>
Co-authored-by: veeck <michael.veeck@nebenan.de>
Co-authored-by: Paranoid93 <6515818+Paranoid93@users.noreply.github.com>
Co-authored-by: Brian O'Connor <btoconnor@users.noreply.github.com>
Co-authored-by: WallysWellies <59727507+WallysWellies@users.noreply.github.com>
Co-authored-by: Jason Stieber <jrstieber@gmail.com>
Co-authored-by: jargordon <50050429+jargordon@users.noreply.github.com>
Co-authored-by: Daniel <32464403+dkallen78@users.noreply.github.com>
Co-authored-by: Ryan Williams <65094007+ryan-d-williams@users.noreply.github.com>
Co-authored-by: Panagiotis Skias <panagiotis.skias@gmail.com>
Co-authored-by: Marc Landis <dirk.rettschlag@gmail.com>
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.

4 participants