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

Adding GoAlert Notification #2011

Merged
merged 15 commits into from
Sep 8, 2022

Conversation

mhkarimi1383
Copy link
Contributor

@mhkarimi1383 mhkarimi1383 commented Aug 23, 2022

Description

Adding GoAlert support

according to Alerting From Generic API and it's handler here

Type of change

Please delete any options that are not relevant.

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Screenshot from 2022-08-24 00-03-08

@mhkarimi1383 mhkarimi1383 marked this pull request as draft August 23, 2022 16:49
@mhkarimi1383 mhkarimi1383 marked this pull request as ready for review August 23, 2022 19:35
@mhkarimi1383
Copy link
Contributor Author

Everything Should be fine my reviews and tests will be finished tomorrow :)

@mhkarimi1383
Copy link
Contributor Author

mhkarimi1383 commented Aug 24, 2022

Hello again
json content type was not working with GoAlert correctly (with lots of tests)
So, I migrated to form-data
but form-data and axios are not able work great together but new version of axios has internal support for form-data handling, so I upgraded axios package

more info:
axios form-data

server/notification-providers/goalert.js Outdated Show resolved Hide resolved
src/components/notifications/index.js Outdated Show resolved Hide resolved
mhkarimi1383 and others added 2 commits August 26, 2022 17:05
Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>
Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>
@mhkarimi1383
Copy link
Contributor Author

Since I added upgraded a package and package-lock.json changed I want to know what is the correct version of npm and nodejs just for running npm ci again to solve it.
or someone do it for me.
thanks

@mhkarimi1383 mhkarimi1383 requested a review from Saibamen August 26, 2022 12:57
@chakflying
Copy link
Collaborator

You should be running npm v7+ to use the newer version of the lockfile. The node version should not matter as long as it's 14+.

@mhkarimi1383
Copy link
Contributor Author

mhkarimi1383 commented Aug 28, 2022

using npm 8.18.0 and node v18.8.0
but CI fails

@k3rnelpan1c-dev
Copy link

k3rnelpan1c-dev commented Aug 28, 2022

IIRC Node 14 + NPM 7 (the default NPM version with Node 14 IIRC), does not support lockfiles v2.
The general CI auto-test use a range of Node Version [14..18] but the check-lintersonly runs 14.

Side notes:

  • 14 being a prior LTS is still in maintained till May 2023 (https://nodejs.org/en/about/releases/)
  • 17, which is also in the auto-test range, should probably be dropped due to it only ever been a "current" release with no further development / maintenance.

@chakflying
Copy link
Collaborator

Thanks, I remembered it wrong.

Thing is, CI should still work even with lockfile version mismatch (as it has worked all this time). I think you broke something else when you regenerated that file on MacOS, somehow the linux dependencies are missing. Anyway I think louislam can handle it if this gets merged.

@mhkarimi1383
Copy link
Contributor Author

Thanks, I remembered it wrong.

Thing is, CI should still work even with lockfile version mismatch (as it has worked all this time). I think you broke something else when you regenerated that file on MacOS, somehow the linux dependencies are missing. Anyway I think louislam can handle it if this gets merged.

I'm on ArchLinux using nvm

@mhkarimi1383
Copy link
Contributor Author

node --version
v14.6.0
npm --version
6.14.6

What is the ideal version?

@k3rnelpan1c-dev
Copy link

k3rnelpan1c-dev commented Aug 30, 2022

okay, took another look, and seems as if the main branch is already using a v2 lock-file. so I am surprised that this hasn't pop up earlier 🤔

Edit:

🤦‍♂️ we where looking at the wrong line (like most regular npm users and the lockfile was just a warning not an error):

npm ERR! code EBADPLATFORM
npm ERR! notsup Unsupported platform for fsevents@2.3.2: wanted {"os":"darwin","arch":"any"} > (current: {"os":"linux","arch":"x64"})
npm ERR! notsup Valid OS: darwin
npm ERR! notsup Valid Arch: any
npm ERR! notsup Actual OS: linux
npm ERR! notsup Actual Arch: x64

looks like fsevents@2.3.2 wants the host to be MacOS, yet we on Linux CI and in the container images too.

@mhkarimi1383
Copy link
Contributor Author

okay, took another look, and seems as if the main branch is already using a v2 lock-file. so I am surprised that this hasn't pop up earlier 🤔

Edit:

🤦‍♂️ we where looking at the wrong line (like most regular npm users and the lockfile was just a warning not an error):

npm ERR! code EBADPLATFORM
npm ERR! notsup Unsupported platform for fsevents@2.3.2: wanted {"os":"darwin","arch":"any"} > (current: {"os":"linux","arch":"x64"})
npm ERR! notsup Valid OS: darwin
npm ERR! notsup Valid Arch: any
npm ERR! notsup Actual OS: linux
npm ERR! notsup Actual Arch: x64

looks like fsevents@2.3.2 wants the host to be MacOS, yet we on Linux CI and in the container images too.

How main branch is working?

@k3rnelpan1c-dev
Copy link

I have not the slightest idea, but a couple of pointers that may be worth evaluating if they are the cause:

  • flaky CI (given you are on Arch you could try nuking your current downloaded dependency cache and attempt build to see if that theory applies)
  • some funky dependency change within a sub-dependency (if any of the dependencies uses version ranges or similar)

(given that CI passed before regenerating the lockfile it can be either)

Please note that I am merely a CI enthusiast who likes this project and stumbled over your PR. I have no power here, besides trying to help debugging 🤔

@mhkarimi1383
Copy link
Contributor Author

fixed by switching to node v16

@louislam louislam merged commit 1e5376d into louislam:master Sep 8, 2022
@mhkarimi1383 mhkarimi1383 deleted the goalert-notification branch June 12, 2023 20:07
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.

5 participants