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

Replace request with fetch in ci-cleanup #7547

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ljowen
Copy link
Contributor

@ljowen ljowen commented Mar 11, 2025

What this PR does

Remove request as a dependency, use node's native fetch (introduced node v18)

Test me

ci-deploy action runs successfully

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@ljowen ljowen force-pushed the remove-request-dep branch from 29cb0f7 to fcc782b Compare March 11, 2025 01:38
@ljowen ljowen mentioned this pull request Mar 11, 2025
4 tasks
@@ -19,7 +19,6 @@ gh api /repos/${GITHUB_REPOSITORY}/statuses/${GITHUB_SHA} -f state=pending -f co
# Install some tools we need from npm
npm install -g https://github.com/terriajs/sync-dependencies
npm install -g yarn@^1.19.0
yarn add -W [email protected]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current error looks like the same error that caused #7522.

Can you try to remove the rm yarn.lock on line 34 and see if that re-uses the compatible dependencies specified in TerriaMap's yarn.lock and only updates the incompatible dependencies?

I've put up a fix to pin the types package to the previous version in #7551, but since we now have an easy way to reproduce the current failure I suggest we first fix the CI problem.

Copy link
Contributor Author

@ljowen ljowen Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing L:34 fixed this particular issue issue, I'm not sure the rational for deleting the lockfile @na9da @zoran995 any thoughts? Would this cause issues if for example a deploy introduces a new dependency in terriajs ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so we know keeping yarn.lock at least works for this case. I'm guessing the reason it is deleted is because commit 0ce0fde introduced removing the file, but that was for npm.

I've removed the draft status from #7551 so it can be merged.

I have some other commits related to CI in the PR queue, I'm guessing it would be easiest to cherry-pick them into this PR to get the deploy workflow tested. Commit c38759c uses the yarn that is pre-installed on Github's CI runners, commit 7c50852 adds --frozen-lockfile so package.json and yarn.lock are kept in sync, and commit 4701385 stops the publish action if it somehow runs with an incompatible Node version.

Copy link
Collaborator

@na9da na9da Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rm yarn.lock is intentional so that we can discover compatibility issues with newer versions of dependencies sooner (it is ok for the CI to break). When that happens we either fix the code or pin the version for the dependency in package.json (like @pjonsson has done).

Copy link
Contributor

@pjonsson pjonsson Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I knew nothing about this project, I would first look for some job in TerriaMap for finding those breakages, but if you want the CI to sometimes go red for reasons unrelated to the PR I guess that's your choice.

More recent versions of flexsearch include types so I guess upgrading to those is the long-term solution, my pin is just to unbreak the CI.

@ljowen ljowen force-pushed the remove-request-dep branch from a567065 to fcc782b Compare March 12, 2025 01:15
@ljowen ljowen force-pushed the remove-request-dep branch from fcc782b to 5f2dcc2 Compare March 12, 2025 01:15
@ljowen ljowen requested a review from na9da March 12, 2025 02:56
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.

3 participants