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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve broken builds by updating package-lock and specify node version via .nvmrc #976

Merged
merged 1 commit into from Jun 21, 2022

Conversation

pepopowitz
Copy link
Collaborator

@pepopowitz pepopowitz commented Jun 17, 2022

This PR specifies a node version in a .nvmrc file, and updates the package-lock based on that version. This is intended to fix our current situation of all builds failing for the last ten days 馃槵 馃槄

Why were the builds failing?

TL;DR

A new version of node v16 was released, including a newer version of npm, and we needed to update the package-lock file accordingly.

The journey

Looking through the list of recent workflow runs, it looks like the builds stopped failing between this successful check and this unsuccessful one.

The error message in the failing checks is about the package.json and package-lock.json not being in sync:

image

But I couldn't update the lockfile locally

I tried to re-run npm install locally, to update the package-lock, but there were no changes!!!

This made me suspicious of node/npm versions.

The smoking gun

I noticed a difference between those two checks -- the successful one was using node v16.15.0 & npm v8.5.5, and the failing one was using node v16.15.1 & npm v8.11.0. So, yeah -- different.

I confirmed by updating my local environment to use v16.15.1, and now when I ran npm install, I got an updated package-lock file. That's what's in this PR.

Why the .nvmrc?

For people like me using a node version manager, the .nvmrc is automatically read by local tooling to use the specified version of node. By specifying this from now on, we'll increase the likelihood that we'll spot this kind of difference sooner rather than later.

What about our workflows? Do they read the nvmrc?

No, not yet. Right now, we're specifying a node version in the workflows, and we're specifying it broadly -- 16. This is kind of the reason for these build failures -- the new version of v16 was released at the beginning of this month, and our builds started using it, but likely most of us weren't using that version of node locally, so none of us generated an updated package-lock.

In the future, I think we should update our configs to use the nvmrc file as described in this PR, instead of specifying the version in each workflow. But right now I just want to get things building again, before I go on another week of vacation 馃槃

I could also see us saying that the nvmrc specification should be at the major level, e.g. 16, instead of the patch level like it is in this PR, e.g. 16.5.1. This scenario, of an updated minor/patch node version causing build failures, is uncommon, and there is probably more value in the free updates we get by specifying only the major version. 馃し

Copy link
Contributor

@Ben-Sheppard Ben-Sheppard left a comment

Choose a reason for hiding this comment

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

I'm not a JS focused dev but this is what I would expect to see given the error output and your detailed explanation! In our (Identity team) builds we specify the exact Node version and I do wonder, although you've said this is not ideal, should we for now just set the version to completely remove the chance of a mismatch?

@akeller
Copy link
Member

akeller commented Jun 21, 2022

In the future, I think we should update our configs to use the nvmrc file as actions/setup-node#338, instead of specifying the version in each workflow. But right now I just want to get things building again, before I go on another week of vacation 馃槃

I'm good with this and happy to merge.

@akeller akeller merged commit 1601cbd into main Jun 21, 2022
@akeller akeller deleted the pepopowitz/specify-node-version branch June 21, 2022 14:47
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.

None yet

3 participants