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 CI to conform to a standard template and test all supported, non-EOL LTS Node.js releases #549

Merged
merged 5 commits into from
Nov 29, 2024

Conversation

orangejulius
Copy link
Member

@orangejulius orangejulius commented Nov 27, 2024

This PR includes a bunch of commits made by a script that standardizes as much as possible our CI config across all repositories.

Along the way it also ensures we test all Node.js versions that are an LTS release, not EOL, and currently work with this repository. Any Github Actions that were out of date or used old Node.js versions (checkout and setup-node @v2) are also updated.

For the whosonfirst repository that means Node.js 18 and 20. We need to update better-sqlite3 for Node.js 22 support.

Also, the CI OS version is now hardcoded to ubuntu-22.04. We fooled around with an organization wide CI variable to configure that, but it broke CI in forks and doesn't really help us much, so it's now undone.

If there are any other differences in Github Actions Workflow files, they are also now removed by using a standard template.

Finally, because this repository has a Dockerfile and we just updated the Docker baseimage to support Node.js 18, there is an empty commit to trigger a new major version release.

Connects pelias/pelias#950
Connects pelias/pelias#951

This doesn't really save us much effort and breaks CI on forks.

Connects pelias/pelias#951
We will have to upgrade better-sqlite3 before we can support Node.js 22

This also rewrites our CI config so that all Node.js versions are on one line for ease of future grepping.

Connects pelias/pelias#950
A lot of our repositories have diverged from our intended template, so this copies a templated version over whatever was here before

Connects pelias/pelias#951
BREAKING CHANGE: The Docker baseimage has been updated to use Node.js 18.20.5, so this repository's Docker image will also use it.

pelias/pelias#950
@@ -20,5 +16,6 @@ jobs:
node-version: '${{ matrix.node-version }}'
- name: Run unit tests
run: |
[[ -f ./bin/ci-setup ]] && ./bin/ci-setup
Copy link
Member

Choose a reason for hiding this comment

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

what does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a new line i added to our template so that repos can define CI setup steps in a script. For now it's specifically so that the interpolation repo can (soon) use the same exact template as other repos, while keeping some dependency installs in a script.

I think we could get at least pbf2json to use that same template too, if we want to manage installing go ourselves.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth hiding this behind an npm run ci-setup || true script so the file name and path may be varied in each repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's an option: I was weighing the possibilities when considering the implementation and actually this is the main things I'm looking to iterate on with feedback.

  • First and foremost I'm looking to preserve consistency across Github Workflow files, having them identical across all our repos is a huge time saver when we update things.
  • In the past, we've been pretty good about using npm run as the entrypoint for most scripts, so maintaining that consistency has some value for sure.
  • On the flip side, I was actually thinking of avoiding the npm middleware layer, as it's caused problems in the past (see the signal swallowing in Docker that made us move away from npm start to ./bin/start). Though there is also a new builtin Node.js feature in v22 that could help with that some day, or at least mean theres one less process running
  • Being able to modify the path and filename, personally I don't see when we would need that, let me know if there's a particular case I'm forgetting

Copy link
Member Author

Choose a reason for hiding this comment

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

Gonna merge this for now to unblock other repos but we can change it later if we come up with a better idea

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

LGTM

@orangejulius orangejulius merged commit e9214fb into master Nov 29, 2024
5 checks passed
@orangejulius orangejulius deleted the nodejs-18-and-ci-updates branch November 29, 2024 17:41
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