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: update_helper.js recode with pm2 library #3285

Closed
wants to merge 6 commits into from

Conversation

bugsounet
Copy link
Contributor

Like i have say some few days ago.

  • recode: update_helper.js with pm2 library
  • fix: default config -> updates is a array
  • delete: command-exists library (not used)
  • delete: PM2_GetList() function (not used)
  • add: check updates.length (prevent crash)
  • add: [PM2] tag in log (for better visibility)
  • add: pm2 library

@bugsounet
Copy link
Contributor Author

bugsounet commented Dec 6, 2023

Like I have says this

advantage:

  • we use the pm2 library directly
  • avoids weird returns from child_process.exec when requesting a json format from pm2
  • simplified the code

inconvenient:

  • we have vulnerabilities with axios

@rejas
Copy link
Collaborator

rejas commented Dec 7, 2023

yeah, not sure about that axios vulnarbility from node-ical.... maybe we can add the v1.6.0 manually in our package.json and later remove it once node-ical update is working? would that work?

@sdetweil
Copy link
Collaborator

sdetweil commented Dec 7, 2023

we need node-ical 0.17.0 we aren't using the url ics approach node-ical provides..

@bugsounet
Copy link
Contributor Author

it's not related to node-ical
but it's related to from @pm2/js-api
axios vulnarbility is located there in package.json
an issue/pull request is open there

@bugsounet
Copy link
Contributor Author

yeah, not sure about that axios vulnarbility from node-ical.... maybe we can add the v1.6.0 manually in our package.json and later remove it once node-ical update is working? would that work?

tested and not work because @pm2/js-api force another version in its own package.json

@bugsounet
Copy link
Contributor Author

It's really funny, because a lot of people are using pm2 cli with this problem and it doesn't bother them to install pm2 in global

And naturally, on MagicMirror² documentation, we encourage to install it (there and @sdetweil install/upgrade scripts)

And here, we are looking for a solution
It gives you a headache, doesn’t it?

@rejas
Copy link
Collaborator

rejas commented Dec 13, 2023

The headache I have is that our "Review Dependencies" check will always error out once we merge this. And a check that always fails is worthless.

@@ -41,8 +41,7 @@ _This release is scheduled to be released on 2024-01-01._
- Fix #3256 filter out bad results from rrule.between
- Fix calendar events sometimes not respecting deleted events (#3250)
- Fix electron loadurl locally on Windows when address "0.0.0.0" (#2550)
- Fix updatanotification (update_helper.js): catch error if reponse is not an JSON format (check PM2)
- Fix missing typeof in calendar module
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deletion of line 45 was not intentional, was it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a oversight when i merged develop into it. will merge develop again now that other PRs got merged and re-add it

@bugsounet
Copy link
Contributor Author

So I have 2 solutions:

  • Waiting for an pm2 fix (an issue/PR) is ready for this
    • make a draft of this PR and waiting...

or

  • make a fork of pm2
    • correct this vulnerability
    • publish it into my npm account and use it
    • waiting for original pm2 fix and switch into it in next MM² version

@bugsounet
Copy link
Contributor Author

That you can see my fix works
Note: We don't use @pm2/js-api in this part of code but ... it's on main package.json of pm2

now @rejas what I do ?

  • Publish my own package (pm2 and js-api) to npm and use it temporarily?
  • use it directly from my github account (like actually)
    or
  • waiting for an official fix ? and still this PR in draft ?

@bugsounet
Copy link
Contributor Author

again another solution: recode pm2 and limit it to our own use (for MagicMirror² only)

  • just using: start / stop / restart / list / status / log ... (with no advanced function)
  • it will be automaticaly linked to original pm2 (like actually)

we can call It something like 'MM-pm2` (I'm able to do it)

@sdetweil
Copy link
Collaborator

I would wait. there is no rush for this

as a user I do not want auto update.

@bugsounet
Copy link
Contributor Author

humm... so best way is not code this functionality and close this PR ?

@sdetweil
Copy link
Collaborator

no. wait til the needed support is there.

@@ -87,6 +86,7 @@
"module-alias": "^2.2.3",
"moment": "^2.29.4",
"node-ical": "0.16.1",
"pm2": "https://github.com/bugsounet/pm2#build",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the official package not used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a draft... I test something but i plan to use official packet
I'm working about something else actually. I will see after ;)
Don't worry

@bugsounet
Copy link
Contributor Author

recoded for v2.27.x with #3332

@bugsounet bugsounet closed this Jan 3, 2024
rejas pushed a commit that referenced this pull request Jan 20, 2024
#3332)

#3285

Because there is so many conflit with package,
I have rewrite the code with v2.27.0-develop

For remember:

 * recode: `update_helper.js` with `pm2` library
 * fix: default config -> `updates` is a array
 * delete: `command-exists` library (not used)
 * delete: `PM2_GetList()`  function (not used)
 * add: check `updates.length` (prevent crash)
 * add: `[PM2]` tag in log (for better visibility)
 * add: `pm2` library
 
advantage:
  * we use the pm2 library directly
* avoids weird returns from child_process.exec when requesting a json
format from pm2
  * simplified the code

inconvenient:
  * we have vulnerabilities with axios

240120 Fix:
* use `pm2_env.pm_cwd` instead of `pm2_env.PWD` : prevent using `pm2
restart <id> --update-env` in other directory (for enable GPU rendering
for exemple)
 * resolve packages (again)
@bugsounet bugsounet deleted the pm2 branch January 20, 2024 20:48
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.

None yet

4 participants