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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changeset/lemon-maps-explain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
---
14 changes: 14 additions & 0 deletions .changeset/rotten-squids-act.md
Original file line number Diff line number Diff line change
@@ -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
3 changes: 2 additions & 1 deletion packages/kas/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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)'"
},
Expand All @@ -43,4 +44,4 @@
"algebra",
"symbolic"
]
}
}
3 changes: 2 additions & 1 deletion packages/keypad-context/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -34,4 +35,4 @@
"react": "^18.2.0"
},
"keywords": []
}
}
3 changes: 2 additions & 1 deletion packages/kmath/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -34,4 +35,4 @@
"underscore": "1.4.4"
},
"keywords": []
}
}
6 changes: 4 additions & 2 deletions packages/math-input/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -78,4 +80,4 @@
"react-transition-group": "^4.4.1"
},
"keywords": []
}
}
3 changes: 2 additions & 1 deletion packages/perseus-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {},
Expand All @@ -30,4 +31,4 @@
},
"peerDependencies": {},
"keywords": []
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

OCD-level 10 comment: looks like all the package.json lost their newline at end of file. I'm not sure if that matters, but Github flags it so... 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't make a difference. I just saved like usual - no idea why this means it doesn't get a newline.

3 changes: 2 additions & 1 deletion packages/perseus-editor/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down Expand Up @@ -99,4 +100,4 @@
"underscore": "^1.4.4"
},
"keywords": []
}
}
3 changes: 2 additions & 1 deletion packages/perseus-linter/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -35,4 +36,4 @@
"prop-types": "15.6.1"
},
"keywords": []
}
}
3 changes: 2 additions & 1 deletion packages/perseus/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down Expand Up @@ -124,4 +125,4 @@
"underscore": "^1.4.4"
},
"keywords": []
}
}
3 changes: 2 additions & 1 deletion packages/pure-markdown/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -31,4 +32,4 @@
"devDependencies": {},
"peerDependencies": {},
"keywords": []
}
}
3 changes: 2 additions & 1 deletion packages/simple-markdown/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -37,4 +38,4 @@
"keywords": [
"markdown"
]
}
}
62 changes: 40 additions & 22 deletions utils/internal/pre-publish-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment on lines +31 to +40
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay. Thanks for adding this check here too!

Comment on lines +31 to +40
Copy link
Member Author

Choose a reason for hiding this comment

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

note: New check to make sure all the packages we publish are running our prepublish check to avoid snapshot releases usurping real releases.

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(
Expand All @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: I just changed that nested if in an else to an else if.

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.`,
Expand All @@ -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};
7 changes: 7 additions & 0 deletions utils/package-pre-publish-check.sh
Original file line number Diff line number Diff line change
@@ -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
17 changes: 13 additions & 4 deletions utils/pre-publish-check-ci.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Comment on lines +25 to +38
Copy link
Member Author

Choose a reason for hiding this comment

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

note: This means we check all the packages for all the things before we quit, rather than quitting on the first error. Makes it easier to do changes to the overall checks and ensure compliance.

},
);
8 changes: 8 additions & 0 deletions utils/publish-snapshot.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Loading