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 node version on CI #20287

Merged
merged 7 commits into from Dec 12, 2022
Merged

馃獰馃敡 Fix node version on CI #20287

merged 7 commits into from Dec 12, 2022

Conversation

timroes
Copy link
Collaborator

@timroes timroes commented Dec 9, 2022

What

This aligns our node version handling we use across the product. This is caused by us having had package-lock.json changes due to a missmatch in npm versions run during development.

How

  • Use fixed versions for webapp development instead of the lts shorthands, so we are set to an explicit node version, since the lts version might change over time while new minor releases are incoming.
  • Load the version Gradle downloads for building the webapp and e2e tests from said .nvmrc files, to avoid duplication in where we store this version.
  • Add the version to engines.node in the package.json as an explicit version. This will now cause npm install to fail if someone tries to execute it locally with a version that's not exactly the one we're expecting.
  • Since gradle takes care of downloading the node version for webapp and e2e tests, we don't need to use github actions to load them. I removed the corresponding setup-node actions from the workflow files, to not cause confusion there with missmatching versions.
  • Adjust the package-lock.json files to actually match the now explicitly set npm version... not sure how many times they had a format change during one major version 馃槱

I tried to use the latest LTS support for this, but unfortunately there are still issues with CRA app, that won't work on the latest LTS release, so we're still stuck on LTS Gallium till they fix this: facebook/create-react-app#11708

You'll see there are some setup-node actions left in the workflow files. We use Spotless, which uses prettier in the top level build.gradle file to format JSON and YAML across the repository (except the webapp). Thus the spotless tasks actually needs a locally installed npm version, why the workflows that trigger a full build still require the npm to be installed locally. The version here isn't that important, since it's just using npm once to install prettier, so I set it to the latest lts release. The webapp is not using that npm version, and always will download it's own version via Gradle!

I tried to solve this by downloading that Gradle version for spotless also via the gradle node plugin, but that unfortunately doesn't work, since the spotless plugin required it to be there during Gradle's Configuration phase (while parsing the build.gradle files), but the download task cannot download it before the Execution phase (when tasks are actually executed). So we can't leverage gradle to download npm for Spotless.

Follow up

I want to create a separate PR still that adds a Gradle task that makes the FE build (and e2e test build) fail if the package-lock.json file is changed after running npm install. Since this requires adding custom tasks I prefer having that in a separate PR.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 9, 2022
@timroes timroes temporarily deployed to more-secrets December 9, 2022 10:04 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 10:04 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 10:27 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 10:27 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 12:27 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 12:28 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 12:32 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 12:33 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 13:05 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 13:05 — with GitHub Actions Inactive
@timroes timroes marked this pull request as ready for review December 9, 2022 13:51
@timroes timroes requested review from a team as code owners December 9, 2022 13:51
@timroes timroes temporarily deployed to more-secrets December 9, 2022 14:00 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 9, 2022 14:01 — with GitHub Actions Inactive
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Approving pending build

@josephkmh
Copy link
Contributor

josephkmh commented Dec 9, 2022

I want to create a separate PR still that adds a Gradle task that makes the FE build (and e2e test build) fail if the package-lock.json file is changed after running npm install. Since this requires adding custom tasks I prefer having that in a separate PR.

Do we need gradle for this? Couldn't we just use npm ci instead of npm install? AFAIK that will install from package-lock.json and also throw if there is a mismatch with package.json https://docs.npmjs.com/cli/v9/commands/npm-ci

@timroes
Copy link
Collaborator Author

timroes commented Dec 9, 2022

I can investigate that next week. My understanding was that npm ci will only install from package-lock.json but wouldn't fail on those minor inconsistencies. That's why I decided to go with the change detection mechanism, but I will give this another look.

Copy link
Contributor

@josephkmh josephkmh left a comment

Choose a reason for hiding this comment

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

LGTM! I pulled and tested locally to confirm:

  • npm install failed unless I was using node 16.18.1
  • npm build does not fail when using other versions (although has build errors with node 18 馃槃 )
  • npm run start does not fail when using other versions

@@ -72,7 +72,7 @@ jobs:

- uses: actions/setup-node@v3
with:
node-version: "lts/gallium"
node-version: "lts/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is lts/* ok here? I'm assuming node is just required here (and in other workflows) for some scripting, which is unrelated to any of the frontend build/test steps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only needed for installing prettier from the build.gradle file for formatting JSON/YAML files (outside the webapp folder). The version here does not really matter, since it only needs any npm version to install prettier, so I made sure this is running on the latest lts instead of gallium.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks good to me, should we add a small section to the readme how to keep your local node version in sync using nvm?

@timroes timroes temporarily deployed to more-secrets December 12, 2022 10:57 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 12, 2022 10:58 — with GitHub Actions Inactive
@timroes
Copy link
Collaborator Author

timroes commented Dec 12, 2022

Looks good to me, should we add a small section to the readme how to keep your local node version in sync using nvm?

I'll put something about this into our development setup guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants