-
Notifications
You must be signed in to change notification settings - Fork 11
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
update node to 16 and add configurable install command #45
base: develop
Are you sure you want to change the base?
Conversation
Thanks for the PR! Can you share the use-case for customizing the installation command? Also, ideally dependency upgrades (incl Node) would be in a different PR so it can be discussed and tested in isolation but doesn't seem like a big deal here. Can you set it to Node v14 though? LTS is still active till 2023 April. |
Yeah, absolutely 😊 Our use case is that we are using https://rushjs.io/ for our monorepo. So the command to install the deps would be It could be autodetected, but I thought it would cover more use-cases if the users could select their install command (if they wanted to) I can also split them out in different prs if you prefer that 😊 I went for 16 because recently all the GitHub actions got bumped to 16, but yeah I'll set it to 14 😊 |
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.
Thanks @alexdor !
@@ -7,7 +7,6 @@ async function exec(command, options) { | |||
const startTime = Date.now(); | |||
const exitCode = await _exec(command, null, { | |||
...options, | |||
silent: true, |
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.
Why is this removed?
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.
So that the output of the build/install commands are visible, it makes it a lot easier when smth breaks during build or installation to see what is happening
installCommand = autoDetectInstallCommand(); | ||
} else { | ||
log.info(`Custom install command providing. Installing dependencies with ${installCommand}`); | ||
} |
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.
Let's move this out of the else
block and log it for all cases at L22.
log.info(`Installing dependencies: ${installCommand}`);
async function npmCi({ cwd } = {}) { | ||
async function npmCi({ cwd, installCommand } = {}) { | ||
if (!installCommand) { | ||
installCommand = autoDetectInstallCommand(); |
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.
Let's default this to npx ci
: https://github.com/privatenumber/ci
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.
That package is not easily importable, would it be possible to export the function in a way that can be imported by other libs instead of just it being a self executed script?
@@ -4,7 +4,12 @@ import { rmRF } from '@actions/io'; | |||
import * as log from './log.js'; | |||
import exec from './exec.js'; | |||
|
|||
async function npmCi({ cwd } = {}) { | |||
async function npmCi({ cwd, installCommand } = {}) { |
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.
Can you rename this function to installDependencies
?
@@ -21,6 +21,9 @@ inputs: | |||
description: 'Sort order: desc, asc' | |||
hide-files: | |||
description: 'Glob pattern to hide files with' | |||
install-command: | |||
description: 'Command to install the package with, if empty the action will try to autodetect it' |
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.
description: 'Command to install the package with, if empty the action will try to autodetect it' | |
description: 'The command used to install your project's dependencies. By default, there is autodetect logic based on `package.json#packageManager`, package lock files, etc.' |
Any updates on this? |
I need this one too because I want to use |
@JounQin I've been waiting for this to be merged because the current version of the package isn't usable from other libraries until @privatenumber has had some time to look through things you can use |
@alexdor Thanks, I have another proposal at #49, and it should be implemented at https://github.com/privatenumber/ci, I'll raise a PR for it soon. |
Thanks for sharing your use-cases. Using |
No description provided.