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 ci_args to npm ci run cmd #33

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add ci_args to npm ci run cmd #33

wants to merge 1 commit into from

Conversation

camallen
Copy link
Contributor

@camallen camallen commented Oct 5, 2022

fixes #32 allow us to pass explicit args to npm ci cmd, e.g. npm ci --legacy-peer-deps

I wasn't sure how to best handle this with say an optional boolean input (default to false) that turns on this specific behaviour to limit the scope of what can be passed to npm ci cmd (non trivial and complicated the action setup. So i chose the allow any flag to flow through via the args string input which will allow us to morph the npm ci behaviours as required. Happy to revisit if folks think this is too open

details on the args we can pass to npm ci

allow us to pass explicit args to npm ci cmd, e.g. `npm ci --legacy-peer-deps`
@eatyourgreens
Copy link
Contributor

Looks good to me. I wonder if we should enable --ignore-scripts by default? Pre- and post-install scripts can build and install binaries, which makes them hard to secure.

@camallen
Copy link
Contributor Author

camallen commented Oct 5, 2022

Looks good to me. I wonder if we should enable --ignore-scripts by default? Pre- and post-install scripts can build and install binaries, which makes them hard to secure.

I think that's a good idea, i've proposed that in #34 and if folks think that is useful i'll rebase this over that change before merging.

Copy link
Member

@zwolf zwolf left a comment

Choose a reason for hiding this comment

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

Revisiting this and it looks good. At the moment, there's no test associated with the "NPM Build" action. Could you throw a quick one together, either pulled from a test repo or self-contained? It just needs to be the absolute minimum required to allow the shared workflow to run the build command and save the artifact. Happy to help get it going, I'm just not sure what the best way to test the barest-bones npm repo is--check out the other two tests to see what I mean.

With simple test included, we can ensure that changes to the workflow (syntax, versions, stuff like this) work within the context of the action before merging. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPM builds: allow for legacy peer dependencies
3 participants