-
Notifications
You must be signed in to change notification settings - Fork 359
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
build: improve update script error handling #832
build: improve update script error handling #832
Conversation
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.
LGTM. I wonder if the script could also check if nvm
is installed and if not, install it and check if the system node is set to lts
, otherwise bail
@lmiller1990 you mind taking a look at this PR? the Node v19 job for Cypress 9 is failing since the actual |
The results of .github/workflows/example-node-versions.yml are flaky for multiple PRs. In the run for this PR https://github.com/cypress-io/github-action/actions/runs/4429718318 https://github.com/cypress-io/github-action/actions/runs/4429718318/jobs/7770543697 fails with
which doesn't quite make sense because this is an Ubuntu runner, not a https://github.com/cypress-io/github-action/actions/runs/4429718318/jobs/7770541047 succeeds, although there shouldn't be any difference between this run and the previous one which failed;
I'm not sure if this is a GitHub issue or a Node.js issue at the moment. It could be a flaky node or load-balancer for instance. If it's still happening tomorrow I would take the problem to GitHub / Node.js support to look at.
|
I think its a Node.js issue. When I actually try to download the file itself from the browser it fails with a 404. Almost seems like a faulty load balancer or something of sorts. If it is passing for the Cypress latest v19 job, I would assume the load balancer. My guess is this should be fixed either by a node version bump or someone adjusting the load balancer in the near future. I wouldn't worry too much about it, but as you said if it is still happening tomorrow it might be worth looking at, but I don't know how we would be able to fix that besides filing an issue. |
@AtofStryker |
🎉 This PR is included in version 5.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR improves the error handling for the update scripts called by
It was previously possible for them to fail silently, particularly if
pnpm
oryarn
were not installed. This situation can occur ifnvm
is used to manageNode.js
versions, since global installations ofpnpm
andyarn
are tied to a specificNode.js
version and not carried over, for instance ifNode.js
is updated to the latest LTS version.The scripts now check that the following commands are available:
and advise installation if they are missing.
Verification
On Ubuntu and Windows 11 Pro.
npm
available (for instance by uninstallingnvm
).npm -v
should produce "command not found", executeand confirm output of message to install
Node.js
ornvm
.nvm
.On Windows execute
Execute
Confirm that the script informs that
npm
is installed, and thatpnpm
andyarn
are not installed.pnpm
and execute the script again.Confirm that the script informs that
npm
is installed, and thatyarn
is not installed.yarn
and execute the script again.This time the script should run to completion and update all examples to use
cypress@12.8.0
(excepting those in the examples/v9 directory).