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

fix(ci): Fix for the Actions failing on Windows Runner #4557

Merged
merged 10 commits into from
Mar 20, 2024
Merged

fix(ci): Fix for the Actions failing on Windows Runner #4557

merged 10 commits into from
Mar 20, 2024

Conversation

Zaid-maker
Copy link
Contributor

@Zaid-maker Zaid-maker commented Mar 6, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Adds a --force flag which fixes CI failing source

Seems like there is node-gyp issue due to drop of support of Node 14 which is preventing the CI to install global NPM version, removing the npm i -g npm@9 fix the ci and seems OK Bumping to Node 16 fixes the issue

Tested the same method on my Repo CI it seems to run OK not the issue with NPM or Actions, i cannot figure out the issue.

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

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 generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

@Zaid-maker Zaid-maker marked this pull request as draft March 6, 2024 14:33
@chakflying
Copy link
Collaborator

I wonder if there is a way to avoid having to touch the global npm version.

@Zaid-maker
Copy link
Contributor Author

Zaid-maker commented Mar 6, 2024

I wonder if there is a way to avoid having to touch the global npm version.

I guess we have to remove the npm i -g npm@9 step that's the only fix, i pushed lets see what happened

@Zaid-maker
Copy link
Contributor Author

yeah removing that fix

@CommanderStorm
Copy link
Collaborator

I wonder if there is a way to avoid having to touch the global npm version

npm versions dependent on the node version.
While this may be changing with corepack, I see this somewhat unlikely.

Do you have a clue why this was added?

Having a look at the node-gyp-releasees, they have dropped node 14.
Could bumping to node 16 solve this issue?

@Zaid-maker
Copy link
Contributor Author

Zaid-maker commented Mar 6, 2024

We could see by bumping to Node 16 for testing if everything goes correct then we should adopt that

Could bumping to node 16 solve this issue?

yes bumping to node.js 16 fix

@Zaid-maker Zaid-maker marked this pull request as ready for review March 6, 2024 15:51
@Zaid-maker
Copy link
Contributor Author

So this change can stay?

@CommanderStorm
Copy link
Collaborator

As you noted above both

  • dropping npm i -g npm@9 and
  • bumping node to v16 works for resolving this issue.

I would vote for changing the node version to >=v16.
The rationale is that we will have to do that anyway due to #4414
@chakflying @louislam any objections?

Another option would be bumping node to >=v18, which would allow for bugfixes like croner@8.0.0 for AKDT timezones not triggering.
I don't know how our decision in #3747 figures into this.

However, bumping to node >= v16 this requires more files to be changed:

@chakflying
Copy link
Collaborator

I have no objection to moving to >=16.

But also, it's a bit funny that a failing CI on Windows is the last straw on the camel's back for dropping 14.

@Zaid-maker
Copy link
Contributor Author

Zaid-maker commented Mar 7, 2024

Only bumping to Node 16 resolves this issue, the npm i -g npm@9 still there.

And maybe we can keep open this and wait for 2.0.x release and give a instructions on release we drop 14, 16..etc and here's the guide on how to upgrade which i think make a significant improvement in Security and other aspects.

Still upgrading means more bugfixes and happyy Users, decision is on Louis now

@louislam
Copy link
Owner

louislam commented Mar 7, 2024

Um... Maybe I have to rethink to drop Node.js 14 and 16 in 2.0.0, because these weird issues I believe will eventually happen again and again.

I just feel Node.js' LTS versions are somehow too short. 18 is going to reach eol next year.

@Zaid-maker
Copy link
Contributor Author

I just feel Node.js' LTS versions are somehow too short. 18 is going to reach eol next year.

yes you are right

image

@CommanderStorm CommanderStorm marked this pull request as draft March 7, 2024 13:49
@CommanderStorm CommanderStorm added this to the 2.2.0 milestone Mar 14, 2024
@chakflying
Copy link
Collaborator

Can we have an interim fix by dropping npm i -g npm@9 such that CIs can be passing again?

@Zaid-maker
Copy link
Contributor Author

Zaid-maker commented Mar 15, 2024

Can we have an interim fix by dropping npm i -g npm@9 such that CIs can be passing again?

Yes but on windows runner needs to be on node 16 other runners works fine.

Node-gyp has issue on node 14 on windows runner

@chakflying
Copy link
Collaborator

If this is the only way to get it working then we should adopt it, and merge to master ASAP. I don't think we should get use to CI checks failing on PRs and commits.

@Zaid-maker
Copy link
Contributor Author

If this is the only way to get it working then we should adopt it, and merge to master ASAP. I don't think we should get use to CI checks failing on PRs and commits.

Yup you are right.

@CommanderStorm CommanderStorm removed this from the 2.2.0 milestone Mar 20, 2024
@CommanderStorm CommanderStorm added this to the 2.0.0 milestone Mar 20, 2024
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Mar 20, 2024

Maybe I have to rethink to drop Node.js 14 and 16 in 2.0.0

I think we should merge #3747 instead.
I have updated said PR to make it mergable into master

I don't think merging this (half-baked) migration to node 16 is a good idea ^^

@chakflying
Copy link
Collaborator

My perspective is that fixing the CI should have a higher priority than the discussion of dropping v14/16. But if #3747 is functionally equivalent except the documentation change, I'm willing to go with that instead.

@louislam
Copy link
Owner

I merge this first

@louislam louislam marked this pull request as ready for review March 20, 2024 14:20
@louislam louislam merged commit 45690a2 into louislam:master Mar 20, 2024
17 checks passed
@Zaid-maker Zaid-maker deleted the fix-ci branch March 20, 2024 14:45
@CommanderStorm CommanderStorm mentioned this pull request Mar 26, 2024
7 tasks
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