-
Notifications
You must be signed in to change notification settings - Fork 77
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
Upgrade action to Node v16 and actions/core to v1.10 #68
Changes from all commits
74b17c0
cd54bcc
e2ee35c
8cc8e27
ddae832
1631f87
3f09b5f
f0817db
8395ea5
8d97f0f
d5e5236
2a4c66f
08faf4c
9bce14f
cfaedeb
f2c1984
721a57b
6f0090a
8b7eb6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,5 +61,5 @@ outputs: | |
description: The version number that was previously published to NPM | ||
|
||
runs: | ||
using: node12 | ||
using: node16 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⭐ |
||
main: dist/index.js |
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,16 +40,14 @@ | |
"test": "mocha && npm run lint", | ||
"coverage": "nyc -x test -x dist/sourcemap-register.js node_modules/mocha/bin/mocha", | ||
"upgrade": "npm-check -u && npm audit fix", | ||
"bump": "bump --tag --push --all && git tag -afm v1 v1 && git push --tags --force", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this script removed? It looks like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been so long since I looked at this I can't remember. Maybe an issue I hit when trying to develop on a fork if I had to guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these scripts were calling out to https://github.com/JS-DevTools/version-bump-prompt. I think it's fine to remove the scripts (and the dev dep below on
|
||
"release": "npm run clean && npm run build && npm test && npm run bump", | ||
"_eslint": "eslint \"**/*.@(js|ts)\"", | ||
"_prettier": "prettier \"**/*.@(js|ts|json|md|yml)\"" | ||
}, | ||
"engines": { | ||
"node": ">=16" | ||
}, | ||
"devDependencies": { | ||
"@actions/core": "^1.2.6", | ||
"@actions/core": "^1.10.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⭐ |
||
"@jsdevtools/chai-exec": "^2.1.1", | ||
"@jsdevtools/eslint-config": "^1.1.4", | ||
"@jsdevtools/version-bump-prompt": "^6.1.0", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
import { debug, getInput, setFailed, setOutput } from "@actions/core"; | ||
import { | ||
debug, | ||
getInput, | ||
setFailed, | ||
setOutput as setOutputActionsCore, | ||
} from "@actions/core"; | ||
import { npmPublish } from "../npm-publish"; | ||
import { Access, Options } from "../options"; | ||
|
||
|
@@ -62,6 +67,17 @@ async function main(): Promise<void> { | |
} | ||
} | ||
|
||
/** | ||
* Set output with logging to stdout for test support | ||
*/ | ||
function setOutput(...args: Parameters<typeof setOutputActionsCore>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I needed this to make the tests backwards compatible with how the new setOutput works. If things are working without it now maybe we don't need it anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my testing it seems like we don't need it locally, but we almost certainly need it in CI. Let's keep it! Otherwise we'd have to fiddle with the Later, I'd like to refactor the tests to avoid calling |
||
if (process.env.NODE_ENV === "test") { | ||
console.log(`TEST::set-output name=${args[0]}::${args[1]}`); | ||
return; | ||
} | ||
mcous marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return setOutputActionsCore(...args); | ||
} | ||
|
||
/** | ||
* Prints errors to the GitHub Actions console | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ export async function npmPublish(opts: Options = {}): Promise<Results> { | |
|
||
let results: Results = { | ||
package: manifest.name, | ||
registry: options.registry, | ||
mcous marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⭐ But could be moved to other PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a consistent change, given that we may use a default value for |
||
// The version should be marked as lower if we disallow decrementing the version | ||
type: | ||
(options.greaterVersionOnly && cmp === -1 && "lower") || diff || "none", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ const { EOL } = require("os"); | |
|
||
describe("GitHub Action - failure tests", () => { | ||
it("should fail if the NPM token isn't set", () => { | ||
files.create([{ path: "workspace/not-package.json", contents: {} }]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⭐ I think this is important, but can't remember why anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing this line has no effect on the tests for me. Maybe it isn't needed anymore? |
||
|
||
let cli = exec.action({ | ||
env: { | ||
INPUT_TOKEN: "", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐