Skip to content

add ci_args to npm ci run cmd - #33

Open
camallen wants to merge 1 commit into
mainfrom
add-ci-args
Open

add ci_args to npm ci run cmd#33
camallen wants to merge 1 commit into
mainfrom
add-ci-args

Conversation

@camallen

@camallen camallen commented Oct 5, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

camallen commented Oct 5, 2022

Copy link
Copy Markdown
Contributor Author

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.

@zwolf zwolf left a comment

Copy link
Copy Markdown
Member

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