CenterForDigitalHumanities / CenterForDigitalHumanities/TinyNode

CD workflows use `npm install` instead of `npm ci`, so deploys can drift from the lockfile

Open
#130 0 comments 0 reactions 0 assignees View on GitHub
dependencies enhancement
Dominant language
JavaScript
Stars
1
Forks
3
Avg merge
29m
Merged PRs (30d)
1

Description

## Summary

`tests.yaml` installs with `npm ci`, but all four install steps in the CD workflows use `npm install`. `npm ci` installs exactly what `package-lock.json` pins; `npm install` is free to resolve newer in-range versions and rewrite the lockfile. The result is that the dependency tree CI validates is not necessarily the tree that runs on vlcdhp02 or vlcdhprdp02.

## Affected steps

| File | Line | Job | Runs on | Needs devDeps? |
|------|------|-----|---------|----------------|
| `.github/workflows/cd_dev.yaml` | 55 | `test` | `ubuntu-latest` | yes (`npm run allTests`) |
| `.github/workflows/cd_dev.yaml` | 84 | `deploy` | `vlcdhp02` | no |
| `.github/workflows/cd_prod.yaml` | 20 | `test` | `vlcdhprdp02` | yes (`npm run allTests`) |
| `.github/workflows/cd_prod.yaml` | 46 | `deploy` | `vlcdhprdp02` | no |

For contrast, `tests.yaml:27` and `tests.yaml:47` already do this correctly.

## Why it matters

**1. Deploys are not reproducible.** A caret range like `^17.4.2` lets the server resolve a different version at deploy time than CI tested. This directly undercuts lockfile work — #128 pins a tree, verifies it passes `npm audit`, `npm-check`, and the full suite, and then the deploy step is free to install something else.

**2. Silent drift stays silent.** `npm ci` fails loudly when `package.json` and `package-lock.json` disagree. `npm install` quietly reconciles them. A stale lockfile can therefore ride along unnoticed — which is exactly what happened to the lockfile root `version` field, stuck at `0.1.1` on `main` while `package.json` said `1.0.0` until #128 regenerated it.

**3. `cd_prod.yaml` runs both jobs against the same directory on the same box.** The `test` job does `npm install` in `/srv/node/tiny-node/`, then the `deploy` job does `git stash && git checkout main && git pull && npm install` in that same directory. If the first `npm install` rewrites `package-lock.json`, the working tree is now dirty and the `git stash` masks it rather than surfacing it. With `npm ci` the lockfile is never written, so there is nothing to stash.

**4. The dev cache is already configured for `npm ci`.** `cd_dev.yaml:41-52` caches `~/.npm` keyed on `hashFiles('**/package-lock.json')`. That is the npm download cache, which is precisely what `npm ci` consumes — so this swap costs nothing on the hosted runner and arguably speeds it up.

## Proposed change

Replace `npm install` with `npm ci` in all four places. Minimal and behavior-preserving:

```yaml
# .github/workflows/cd_dev.yaml:53-56
- name: Install dependencies and run the test
run: |
npm ci
npm run allTests
```

```yaml
# .github/workflows/cd_prod.yaml:14-21
- name: Test the app on the server
run: |
cd /srv/node/tiny-node/
git stash
git checkout main
git pull
npm ci
npm run allTests
```

The two deploy steps (`cd_dev.yaml:84`, `cd_prod.yaml:46`) take the same `npm ci` swap.

### Optional refinement, with a tradeoff

The deploy steps only need runtime dependencies, so `npm ci --omit=dev` would work there and would keep `playwright`, `c8`, and `supertest` off the production server. The cost: after a deploy, running `npm run allTests` by hand on that box fails until someone reinstalls devDeps. Since `cd_prod.yaml` runs its test job in the same directory, the next pipeline run would reinstall them anyway — but manual on-server testing between deploys would break. Worth deciding deliberately rather than folding into this change by default.

## Caveats

- `npm ci` deletes `node_modules` before installing, so it is slower than a warm `npm install` on the self-hosted boxes. The reproducibility is the point, but the added minute per deploy is real.
- `npm ci` hard-requires `package-lock.json` to be present and in sync. That is the desired behavior, but it means a future PR that edits `package.json` without regenerating the lock will now fail CD instead of silently self-correcting.

## Context

Found during static review of #128. Related to #124, which covers the other reproducibility gap in these same CD workflows (actions pinned to `@master` rather than a release tag).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.