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

Update and Enable GitHub Actions Tests #2

Closed
wants to merge 5 commits into from
Closed

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Jan 6, 2025

This PR ensures we can run tests on this repository.

Also ensuring we update how we run our tests to ensure things will work within Pulsar on our current version of NodeJS.


EDIT:

Currently this repo is unable to install properly to run it's tests. This is not a fault of CI, but instead the fault of our package-lock.json file being so far out of date. Refer to DeeDeeG's comments for a much better explanation. This PR will leave things broken here, but will rely on further PRs to fix and resolve those issues.

This was referenced Jan 6, 2025
@DeeDeeG
Copy link
Member

DeeDeeG commented Jan 6, 2025

Looks good over all. 👍 Thanks for doing this!

@DeeDeeG
Copy link
Member

DeeDeeG commented Jan 6, 2025

TL;DR: The true underlying fix (for npm install not working), is updating git-utils and nan, whatever way one does do so it should work.

Optional details below:


W.R.T. the yarn install vs npm install, it looks like the package-lock.json is super old. Maybe some locked dependencies in there are causing the compile errors? It's also lockfile v1, and newer npm tries to convert that to lockfile v2 but is super bad doing it.

I can recommend one or more of the following

  • Update at least git-utils and nan in the existing package-lock.json OR
  • Delete existing package-lock.json and either:
    • Commit a new/fresh package-lock.json OR
    • Commit a yarn.lock if using yarn install in CI

Apparently from some investigation, the outdated dependencies (that don't compile against Node 16) are git-utils and nan. The following patch allows npm install to work with modern (modernish) Node 16.

Diff for package-lock.json to get npm install working, click to expand if you want:
diff --git a/package-lock.json b/package-lock.json
index c5988dc..4f98d39 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -294,12 +294,12 @@
       "dev": true
     },
     "git-utils": {
-      "version": "5.6.0",
-      "resolved": "https://registry.npmjs.org/git-utils/-/git-utils-5.6.0.tgz",
-      "integrity": "sha512-eOBROJEQPQtkqzZe5V0m43YhKjhmzXTqULxlhoaCwORClnHtZIwkJQTS6THXRbvIqDq/cRHta85IqTbVzdvB5g==",
+      "version": "5.7.3",
+      "resolved": "https://registry.npmjs.org/git-utils/-/git-utils-5.7.3.tgz",
+      "integrity": "sha512-in1hjFfmzY86gKBt+YMTaVyCGtX2WTnN0uPj37bI5HsrnU2oj8OFcWOEzOI5PxQXPMxFxtvRebOHAOGB8M125w==",
       "requires": {
         "fs-plus": "^3.0.0",
-        "nan": "^2.0.0"
+        "nan": "^2.14.2"
       }
     },
     "glob": {
@@ -753,9 +753,9 @@
       }
     },
     "nan": {
-      "version": "2.14.0",
-      "resolved": "https://registry.npmjs.org/nan/-/nan-2.14.0.tgz",
-      "integrity": "sha512-INOFj37C7k3AfaNTtX8RhsTw7qRy7eLET14cROi9+5HAVbbHuIWUHEauBv5qT4Av2tWasiTY1Jw6puUNqRJXQg=="
+      "version": "2.22.0",
+      "resolved": "https://registry.npmjs.org/nan/-/nan-2.22.0.tgz",
+      "integrity": "sha512-nbajikzWTMwsW+eSsNm3QwlOs7het9gGJU5dDZzRTQGk03vyBOauxgI4VakDzE0PtsGTmXPsXTbbjVhRwR5mpw=="
     },
     "nopt": {
       "version": "1.0.10",

If the above diff is applied (can save it to a file e.g. patch.diff in the repo dir and run git apply patch.diff) then npm install works.

@confused-Techie
Copy link
Member Author

Thanks a ton for investigating this and providing a patch @DeeDeeG! Amazing insight into how the whole npm magic works.

My intention was to happily migrate to yarn as that's what we already use over in Pulsar. But do you think there's any benefit to instead fixing npm and refraining from using yarn here? Otherwise I do think there's some value in having cohesive install strategies across repos, but if there's a benefit I'm not seeing I'm not very attached to that cohesiveness

@confused-Techie
Copy link
Member Author

I've moved this PR back to using npm install.

This does mean in it's current state the repo's CI is broken and can not run.

I'll go ahead and rely on #3 to go ahead and resolve the errors within the repo to fix things up, as this PR is purely CI only.

Thanks for all the help @DeeDeeG!

@confused-Techie
Copy link
Member Author

Replaced by #5

@confused-Techie confused-Techie deleted the fix-ci branch January 7, 2025 01:53
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.

2 participants