-
-
Notifications
You must be signed in to change notification settings - Fork 961
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
chore: add node version check to test/e2e/run.sh #2745
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2745 +/- ##
==========================================
- Coverage 75.14% 75.14% -0.01%
==========================================
Files 294 293 -1
Lines 17068 16908 -160
==========================================
- Hits 12826 12705 -121
+ Misses 3266 3232 -34
+ Partials 976 971 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change itself works, could be maybe bit simpler by grabbing and checking only major version but I have a question to GHA running e2e
https://github.com/ory/kratos/blob/master/.github/workflows/ci.yaml#L109
it is using ubuntu-latest, won't this cause problems? I don't see nodejs version pinning there?
@piotrmsc, yeah, I tried a lot of stuff to just get the major version, but couldn't figure out how to get regex capture groups working in bash. If you have an idea, lmk.
I think it is using the LTS version, which is 16.17, currently. But yes, for the time being, we should also pin the version somehow. I'll look into it. |
@piotrmsc I have cleaned up the check in the script and pinned the node version to 16 in the workflow file. Could you take another look? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works, but general question: maybe it's better to just adjust the scripts to work with newer versions of node?
Because that will require updating a bunch of dependencies in a bunch of projects (React, React Native, express examples) and then hope that they fixed the issue downstream. There is just too much going on to tell if this is an easy fix or if it will take longer. This change is just to save everyone some headache if they forget to change the Node version to 16. |
resolves #2738
This PR adds a (temporary) version check to the e2e run script to avoid timeout issues starting with node version 17 and up.
Related issue(s)
#2738
Checklist
If this pull request addresses a security. vulnerability,
I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
Further Comments