Add prepublish checks to block releases of non-snapshot versionsfirst one (#2037)

somewhatabstract · web-flow · commit b80e7882bf58 · 2024-12-18T18:29:16.000-06:00
## Summary: This PR implements a simple preventative measure for the race condition described by https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/3571646568/Race+condition+breaks+Perseus+release whereby an in progress release triggered via merging a changesets PR and a snapshot release triggered by a PR update can lead to a snapshot releasing an actual versioned release. The events that cause this look something like: 1. Version Packages merged, updating versions in `main` 2. Someone updates their PR with a merge of `main`, updating its versions to the ones in `main` that are not yet published 3. The PR action to publish a snapshot runs, but the main release isn't done yet so the new releases aren't present in NPM 4. The snapshot release tries to publish those packages before the main release has tried This change should prevent that last step; failing the snapshot release. We could look at trying other ways to prevent this, such as making the `release.yml`` workflow responsible for snapshot releases too and limit concurrency. However, even that won't fully prevent this since the merge of the "Version Packages" PR does not guarantee the run order of the release workflow and the snapshot workflow. So, this helps prevent the incorrect publish occurring without impacting the official release process - only PR snapshot releases get affected in the cases where this might occur (which seem to be rare, for now). This update also modifies our pre-publish checks to look for all errors before quitting, instead of quitting on the first one. This is a quality of life change for devs that modify this script and need to check all packages are passing without having to run, then fix, then run, then fix, repeatedly. Issue: XXX-XXXX ## Test plan: I ran `SNAPSHOT_RELEASE=1 npm publish --dry-run` on a package that had a non-snapshot release version, and it failed as expected. I also ran it on a package that had the correct `0.0.0-PR...` format, and it succeeded. I also ran a `npm publish --dry-run` on a package without the `SNAPSHOT_RELEASE` env var, and it succeeded. Author: somewhatabstract Reviewers: jeremywiebe, somewhatabstract, jandrade Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x) Pull Request URL: #2037
diff --git a/.changeset/lemon-maps-explain.md b/.changeset/lemon-maps-explain.md
@@ -0,0 +1,2 @@
+---
+---
diff --git a/.changeset/rotten-squids-act.md b/.changeset/rotten-squids-act.md
@@ -0,0 +1,14 @@
+---
+"@khanacademy/kas": patch
+"@khanacademy/keypad-context": patch
+"@khanacademy/kmath": patch
+"@khanacademy/math-input": patch
+"@khanacademy/perseus": patch
+"@khanacademy/perseus-core": patch
+"@khanacademy/perseus-editor": patch
+"@khanacademy/perseus-linter": patch
+"@khanacademy/pure-markdown": patch
+"@khanacademy/simple-markdown": patch
+---
+
+Nothing has changed, but our action requires a changeset per package and I don't know how to do an infrastructure update like this and pass that check
diff --git a/packages/kas/package.json b/packages/kas/package.json
@@ -22,6 +22,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "gen:parsers": "node src/parser-generator.ts",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
@@ -43,4 +44,4 @@
         "algebra",
         "symbolic"
     ]
-}
+}
diff --git a/packages/keypad-context/package.json b/packages/keypad-context/package.json
@@ -22,6 +22,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {
@@ -34,4 +35,4 @@
         "react": "^18.2.0"
     },
     "keywords": []
-}
+}
diff --git a/packages/kmath/package.json b/packages/kmath/package.json
@@ -21,6 +21,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {
@@ -34,4 +35,4 @@
         "underscore": "1.4.4"
     },
     "keywords": []
-}
+}
diff --git a/packages/math-input/package.json b/packages/math-input/package.json
@@ -36,7 +36,9 @@
     "files": [
         "dist"
     ],
-    "scripts": {},
+    "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh"
+    },
     "dependencies": {
         "@khanacademy/keypad-context": "^1.0.9",
         "@khanacademy/perseus-core": "3.0.3",
@@ -78,4 +80,4 @@
         "react-transition-group": "^4.4.1"
     },
     "keywords": []
-}
+}
diff --git a/packages/perseus-core/package.json b/packages/perseus-core/package.json
@@ -22,6 +22,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {},
@@ -30,4 +31,4 @@
     },
     "peerDependencies": {},
     "keywords": []
-}
+}
diff --git a/packages/perseus-editor/package.json b/packages/perseus-editor/package.json
@@ -31,6 +31,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {
@@ -99,4 +100,4 @@
         "underscore": "^1.4.4"
     },
     "keywords": []
-}
+}
diff --git a/packages/perseus-linter/package.json b/packages/perseus-linter/package.json
@@ -22,6 +22,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {
@@ -35,4 +36,4 @@
         "prop-types": "15.6.1"
     },
     "keywords": []
-}
+}
diff --git a/packages/perseus/package.json b/packages/perseus/package.json
@@ -37,6 +37,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {
@@ -124,4 +125,4 @@
         "underscore": "^1.4.4"
     },
     "keywords": []
-}
+}
diff --git a/packages/pure-markdown/package.json b/packages/pure-markdown/package.json
@@ -22,6 +22,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {
@@ -31,4 +32,4 @@
     "devDependencies": {},
     "peerDependencies": {},
     "keywords": []
-}
+}
diff --git a/packages/simple-markdown/package.json b/packages/simple-markdown/package.json
@@ -22,6 +22,7 @@
         "dist"
     ],
     "scripts": {
+        "prepublishOnly": "../../utils/package-pre-publish-check.sh",
         "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
     },
     "dependencies": {
@@ -37,4 +38,4 @@
     "keywords": [
         "markdown"
     ]
-}
+}
diff --git a/utils/internal/pre-publish-utils.ts b/utils/internal/pre-publish-utils.ts
@@ -2,27 +2,47 @@
  * Pre-publish utilities to verify that our publish will go smoothly.
  */
 
-const checkPublishConfig = ({name, publishConfig, private: isPrivate}) => {
+const checkPublishConfig = ({
+    name,
+    publishConfig,
+    private: isPrivate,
+    scripts,
+}): boolean => {
+    let returnCode = true;
+
     // first check if is marked as public and there's access to publish the current package
     if (!publishConfig || (!isPrivate && publishConfig.access !== "public")) {
         const requiredAccessType = isPrivate ? "restricted" : "public";
 
         console.error(
             `ERROR: ${name} is missing a "publishConfig": {"access": "${requiredAccessType}"} section.`,
         );
-        process.exit(1);
+        returnCode = false;
     }
 
     // also check if is marked as private and there's restricted access defined
     if (isPrivate && publishConfig.access !== "restricted") {
         console.error(
             `ERROR: ${name} is marked as private but there is a "publishConfig": {"access": "public"} section already defined. Please change it to "access": "restricted" or remove "private": true to make the package public.`,
         );
-        process.exit(1);
+        returnCode = false;
     }
+
+    // check that we are running our pre-publish check for this package
+    if (
+        !scripts.prepublishOnly ||
+        !scripts.prepublishOnly.includes("utils/package-pre-publish-check.sh")
+    ) {
+        console.error(
+            `ERROR: ${name} must have a "prepublishOnly" script that runs "utils/package-pre-publish-check.sh".`,
+        );
+        returnCode = false;
+    }
+    return returnCode;
 };
 
-const checkField = (pkgJson, field, value) => {
+const checkField = (pkgJson, field, value): boolean => {
+    let returnCode = true;
     if (Array.isArray(value)) {
         if (!value.includes(pkgJson[field])) {
             console.error(
@@ -32,29 +52,29 @@ const checkField = (pkgJson, field, value) => {
                     .map((value) => JSON.stringify(value))
                     .join(", ")}.`,
             );
-            process.exit(1);
-        }
-    } else {
-        if (pkgJson[field] !== value) {
-            console.error(
-                `ERROR: ${
-                    pkgJson.name
-                } must have a "${field}" set to ${JSON.stringify(value)}.`,
-            );
-            process.exit(1);
+            returnCode = false;
         }
+    } else if (pkgJson[field] !== value) {
+        console.error(
+            `ERROR: ${
+                pkgJson.name
+            } must have a "${field}" set to ${JSON.stringify(value)}.`,
+        );
+        returnCode = false;
     }
+    return returnCode;
 };
 
-const checkMain = (pkgJson) => checkField(pkgJson, "main", "dist/index.js");
+const checkMain = (pkgJson): boolean =>
+    checkField(pkgJson, "main", "dist/index.js");
 
-const checkModule = (pkgJson) =>
+const checkModule = (pkgJson): boolean =>
     checkField(pkgJson, "module", "dist/es/index.js");
 
-const checkSource = (pkgJson) =>
+const checkSource = (pkgJson): boolean =>
     checkField(pkgJson, "source", ["src/index.js", "src/index.ts"]);
 
-const checkPrivate = (pkgJson) => {
+const checkPrivate = (pkgJson): boolean => {
     if (pkgJson.private) {
         console.warn(
             `${pkgJson.name} is private and won't be published to NPM.`,
@@ -64,9 +84,7 @@ const checkPrivate = (pkgJson) => {
     return false;
 };
 
-const checkEntrypoints = (pkgJson) => {
-    checkModule(pkgJson);
-    checkMain(pkgJson);
-};
+const checkEntrypoints = (pkgJson): boolean =>
+    checkModule(pkgJson) && checkMain(pkgJson);
 
 export {checkPublishConfig, checkEntrypoints, checkSource, checkPrivate};
diff --git a/utils/package-pre-publish-check.sh b/utils/package-pre-publish-check.sh
@@ -0,0 +1,7 @@
+#!/usr/bin/env bash
+
+# Check if SNAPSHOT_RELEASE is set and the version does not start with 0.0.0-PR
+if [ "$SNAPSHOT_RELEASE" = "1" ] && ! [[ "$npm_package_version" =~ ^0\.0\.0-PR ]]; then
+    echo "Error: Snapshot publish attempted, but $npm_package_name@$npm_package_version does not match version scheme for snapshots. Publish disallowed."
+    exit 1
+fi
diff --git a/utils/pre-publish-check-ci.ts b/utils/pre-publish-check-ci.ts
@@ -16,16 +16,25 @@ import {
 // eslint-disable-next-line promise/catch-or-return
 fg(path.join(__dirname, "..", "packages", "*", "package.json")).then(
     (pkgPaths) => {
+        let allPassed = true;
         // eslint-disable-next-line promise/always-return
         for (const pkgPath of pkgPaths) {
             // eslint-disable-next-line @typescript-eslint/no-require-imports
             const pkgJson = require(path.relative(__dirname, pkgPath));
 
-            if (!checkPrivate(pkgJson)) {
-                checkPublishConfig(pkgJson);
-                checkEntrypoints(pkgJson);
-                checkSource(pkgJson);
+            if (
+                !checkPrivate(pkgJson) &&
+                !checkPublishConfig(pkgJson) &&
+                !checkEntrypoints(pkgJson) &&
+                !checkSource(pkgJson)
+            ) {
+                allPassed = false;
             }
         }
+
+        // Exit only after we've processed all the packages.
+        if (!allPassed) {
+            process.exit(1);
+        }
     },
 );
diff --git a/utils/publish-snapshot.sh b/utils/publish-snapshot.sh
@@ -18,6 +18,14 @@ MYPATH=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 # ROOT is the root directory of our project.
 ROOT="$MYPATH/.."
 
+# This is used in prepublishOnly hooks to verify that the package is correctly
+# versioned for a snapshot release before proceeding.
+# This is done to catch a race condition where a main release is occurring
+# while a snapshot release is requested, avoiding us publishing packages
+# that we shouldn't be.
+# See https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/3571646568/Race+condition+breaks+Perseus+release
+SNAPSHOT_RELEASE=1
+
 pushd "$ROOT"
 
 verify_env() {
