build: fix make invocation in tools.yml

[Trott]

Be in the correct directory for make lint-md-rollup.

Refs: https://github.com/nodejs/node/runs/4270533399?check_suite_focus=true

[github-actions]

Fast-track has been requested by @Trott. Please 👍 to approve.

[aduh95]

According to SC2103, we should rewrite:

cd tools/lint-md
NEW_VERSION=$(npm outdated --omit=dev --parseable | cut -d: -f4 | xargs)
if [ "$NEW_VERSION" != "" ]; then
  echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV
  rm -rf package-lock.json node_modules && npm install --ignore-scripts
  cd ../..
  make lint-md-rollup
fi

into

NEW_VERSION=$(cd tools/lint-md && npm outdated --omit=dev --parseable | cut -d: -f4 | xargs)
if [ "$NEW_VERSION" != "" ]; then
  echo "NEW_VERSION=$NEW_VERSION" >> "$GITHUB_ENV"
  (cd tools/lint-md && rm -rf package-lock.json node_modules && npm install --ignore-scripts)
  make lint-md-rollup
fi

[Trott]

According to SC2103, we should rewrite:

For reasons I can get into if you want, I'm going with their other remediation suggestion which is to add || exit 1 after each of the cd commands.

[Trott]

According to SC2103, we should rewrite:

For reasons I can get into if you want, I'm going with their other remediation suggestion which is to add || exit 1 after each of the cd commands.

Thinking more about it and looking at the code, I don't think we should do either the subshell or the || exit. Here's why:

GitHub Actions run with set -eo pipefail which means that if any individual shell command fails (like cd foo), then the entire run fails.

Adding || exit to the cd commands but not the other commands raises more questions. (Why aren't we doing that on every single line? The answer is because it's not needed on any of the lines, including the cd lines.)

Using a subshell is similarly unnecessary and unwieldy. It's not bad with the code rewrite you have above, but with the fix for the bug I found, it gets confusing because we need to run npm ci before running npm outdated and without capturing the output of npm ci in NEW_VERSION. There are various ways to do that, but none of them are as readable as:

cd tools/lint-md
npm ci
NEW_VERSION=$(npm outdated --parseable | cut -d: -f4 | xargs)

@aduh95 What do you think?

[aduh95]

If we had been using subshells from the start, it would have been very hard to run make on the wrong directory, which is why I wanted to bring it up. If we are adding more
commands, it may not be worth it anymore, so let’s keep it the way it is.
I agree with you regarding the || exit trickery, my comment was specifically about subshells.

[nodejs-github-bot]

Landed in bad6526