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

[UpdateNotification] Fix pm2 using detection when pm2 script is in MagicMirror root folder #3576

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

bugsounet
Copy link
Contributor

I discover this bug:

When pm2 sh script is on MagicMirror root folder, updatenotification is not able to detect pm2 using

0|MagicMirror  | [2024-10-02 17:23:09.215] [DEBUG]   Version Compare: 2.30.0-develop 2.30.0-develop --> true 
0|MagicMirror  | [2024-10-02 17:23:09.216] [DEBUG]   Status: online 
0|MagicMirror  | [2024-10-02 17:23:09.216] [DEBUG]   PM2 MagicMirror starting  from Path: /home/bugsounet/MagicMirror-dev 
0|MagicMirror  | [2024-10-02 17:23:09.216] [DEBUG]   MagicMirror Path /home/bugsounet/MagicMirror-dev/
0|MagicMirror  | [2024-10-02 17:23:09.216] [DEBUG]   Compare: false
0|MagicMirror  | [2024-10-02 17:23:09.216] [INFO]  updatenotification: [PM2] You are not using pm2 

@khassel khassel merged commit ee98a0c into MagicMirrorOrg:develop Oct 2, 2024
5 of 8 checks passed
@bugsounet bugsounet deleted the updatenotification branch October 11, 2024 09:19
@FrankBlackMG
Copy link

This change didn't work for me since my cwd in PM2 is different from my MagicMirror path. It didn't work previous to the fix either. The change below works for my setup and appears to be a solid way for a process to determine if it's managed by PM2. My only concern is that I'm not sure when unique_id was implemented in PM2.

//if (pm.pm2_env.version === this.version && pm.pm2_env.status === "online" && pm.pm2_env.pm_cwd.includes(`${this.root_path}`)) {

if (process.env.unique_id !== undefined && process.env.unique_id === pm.pm2_env.unique_id) {

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 25, 2024

I think the better path is to put a linked file in the MM folder and have pm2 use that.

see the ln command

otherwise there is no way for us to know its related to MagicMirror

look at the info output from
pm2 info x
where x is the name or number of the app being managed

otherwise I think an optional parm to the updateNotification module is the only other way out..

tell us the name of the pm2 app.

my MMM-Config module also depends on the app path being inside the MM folder.

@FrankBlackMG
Copy link

unique_id is a GUID injected by PM2. It's in the process of MagicMirror where you are making the call to the PM2 API and it's also in the environment section of the pm object (pm.pm2_env.unique_id) when iterating over the list of PM2 managed processes. That seems definitive unless I'm missing something. I do think a linked file would work nicely, but it's something that has to be done rather than just making a simple code change. I dumped the whole env json from each pm object in the PM2 list and you can see all of the package.json vars e.g. name=magicmirror or the repository.url being "https://github.com/MagicMirrorOrg/MagicMirror" so those could be checked as well if you didn't trust the GUID match check.

@bugsounet
Copy link
Contributor Author

@FrankBlackMG Thanks for this purpose, il will inspect

rejas pushed a commit that referenced this pull request Oct 28, 2024
…e or outside MagicMirror root folder (#3605)

This will fix #3576 

@FrankBlackMG: 

I don't use `*env.unique_id` because some others modules can use pm2 too
for starting a service and unique_id is the same (this will make
confusion)
So I check `name` and `pm_id` for found it
@sdetweil sdetweil mentioned this pull request Jan 1, 2025
sdetweil added a commit that referenced this pull request Jan 1, 2025
## [2.30.0] - 2025-01-01

Thanks to: @xsorifc28, @HeikoGr, @bugsounet, @khassel,
@KristjanESPERANTO, @rejas, @sdetweil.

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

### Added

- [core] Add wayland and windows start options to `package.json` (#3594)
- [docs] Add step for npm publishing in release process (#3595)
- [core] Add GitHub workflow to run spellcheck a few days before each
release (#3623)
- [core] Add test flag to `index.html` to pass to module js for test
mode detection (needed by #3630)
- [core] Add export on animation names (#3644)
- [compliments] Add support for refreshing remote compliments file, and
test cases (#3630)
- [linter] Re-add `eslint-plugin-import`now that it supports ESLint v9
(#3586)
- [linter] Re-activate `eslint-plugin-package-json` to lint
`package.json` (#3643)
- [linter] Add linting for markdown files (#3646)
- [linter] Add some handy ESLint rules.
- [calendar] Add ability to display end date for full date events, where
end is not same day (showEnd=true) (#3650)
- [core] Add text to the config.js.sample file about the locale variable
(#3654, #3655)
- [core] Add fetch timeout for all node_helpers (thru undici, forces
node 20.18.1 minimum) to help on slower systems. (#3660) (3661)

### Changed

- [core] Run code style checks in workflow only once (#3648)
- [core] Fix animations export #3644 only on server side (#3649)
- [core] Use project URL in fallback config (#3656)
- [core] Fix Access Denied crash writing js/positions.js (on synology
nas) #3651. new message, MM starts, but no modules showing (#3652)
- [linter] Switch to 'npx' for lint-staged in pre-commit hook (#3658)

### Removed

- [tests] Remove `node-pty` and `drivelist` from rebuilded test (#3575)
- [deps] Remove `@eslint/js` dependency. Already installed with `eslint`
in deep (#3636)

### Updated

- [repo] Reactivate `stale.yaml` as GitHub action to mark issues as
stale after 60 days and close them 7 days later (if no activity) (#3577,
#3580, #3581)
- [core] Update electron dependency to v32 (test electron rebuild) and
all other dependencies too (#3657)
- [tests] All test configs have been updated to allow full external
access, allowing for easier debugging (especially when running as a
container)
- [core] Run and test with node 23 (#3588)
- [workflow] delete exception `allow-ghsas: GHSA-8hc4-vh64-cxmj` in
`dep-review.yaml` (#3659)

### Fixed

- [updatenotification] Fix pm2 using detection when pm2 script is inside
or outside MagicMirror root folder (#3576) (#3605) (#3626) (#3628)
- [core] Fix loading node_helper of modules: avoid black screen, display
errors and continue loading with next module (#3578)
- [weather] Change default value for weatherEndpoint of provider
openweathermap to "/onecall" (#3574)
- [tests] Fix electron tests with mock dates, the mock on server side
was missing (#3597)
- [tests] Fix testcases with hard coded Date.now (#3597)
- [core] Fix missing `basePath` where `location.host` is used (#3613)
- [compliments] croner library changed filenames used in latest version
(#3624)
- [linter] Fix ESLint ignore pattern which caused that default modules
not to be linted (#3632)
- [core] Fix module path in case of sub/sub folder is used and use
path.resolve for resolve `moduleFolder` and `defaultModuleFolder` in
app.js (#3653)
- [calendar] Update to resolve issues #3098 #3144 #3351 #3422 #3443
#3467 #3537 related to timezone changes
- [calendar] Fix #3267 (styles array), also fixes event with both exdate
AND recurrence(and testcase)
- [calendar] Fix showEndsOnlyWithDuration not working, #3598, applies
ONLY to full day events
- [calendar] Fix showEnd for Full Day events (#3602)
- [tests] Suppress "module is not defined" in e2e tests (#3647)
- [calendar] Fix #3267 (styles array, really this time!)
- [core] Fix #3662 js/positions.js created incorrectly

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Michael Teeuw <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Karsten Hassel <[email protected]>
Co-authored-by: Ross Younger <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: Bugsounet - Cédric <[email protected]>
Co-authored-by: jkriegshauser <[email protected]>
Co-authored-by: illimarkangur <[email protected]>
Co-authored-by: vppencilsharpener <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Paranoid93 <[email protected]>
Co-authored-by: Brian O'Connor <[email protected]>
Co-authored-by: WallysWellies <[email protected]>
Co-authored-by: Jason Stieber <[email protected]>
Co-authored-by: jargordon <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Ryan Williams <[email protected]>
Co-authored-by: Panagiotis Skias <[email protected]>
Co-authored-by: Marc Landis <[email protected]>
Co-authored-by: HeikoGr <[email protected]>
Co-authored-by: Pedro Lamas <[email protected]>
Co-authored-by: veeck <[email protected]>
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