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

Added npm ci command #49665

Open
wants to merge 4 commits into
base: devel
from

Conversation

@Bramzor
Copy link

Bramzor commented Dec 7, 2018

SUMMARY

npm ci is a new command that allows to install packages based on the package-lock which is a lot faster and prevents the installation of newer versions of packages and dependancies.
More info: https://blog.npmjs.org/post/171556855892/introducing-npm-ci-for-faster-more-reliable
Solved issue #49664

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

npm

ADDITIONAL INFORMATION
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 7, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 7, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/packaging/language/npm.py:283:7: singleton-comparison Comparison to True should be just 'expr' or 'expr is True'

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/packaging/language/npm.py:283:11: E712 comparison to True should be 'if cond is True:' or 'if cond:'

click here for bot help

@@ -269,6 +280,9 @@ def main():
unsafe_perm=unsafe_perm, state=state)

changed = False
if ci == True:
npm.ci_install()
changed = True
if state == 'present':

This comment has been minimized.

@sjerdo

sjerdo Dec 10, 2018

Should this be updated to elif when using ci mode?

Suggested change Beta
if state == 'present':
elif state == 'present':

This comment has been minimized.

@Bramzor

Bramzor Dec 10, 2018

Yes because if CI is used, it will always install so present/absent is not valid with this call.

Update lib/ansible/modules/packaging/language/npm.py
Sure

Co-Authored-By: Bramzor <bramverdonck@telenet.be>

@Akasurde Akasurde changed the title #49664 Added npm ci command Added npm ci command Dec 19, 2018

@@ -59,6 +59,12 @@
type: bool
default: no
version_added: "2.8"
ci:
description:
- Install packages based on package-lock file, same as running npm ci

This comment has been minimized.

@Akasurde

Akasurde Dec 19, 2018

Member
Suggested change Beta
- Install packages based on package-lock file, same as running npm ci
- Install packages based on package-lock file, same as running npm ci.
@@ -269,6 +280,9 @@ def main():
unsafe_perm=unsafe_perm, state=state)

changed = False
if ci is True:

This comment has been minimized.

@Akasurde

Akasurde Dec 19, 2018

Member
Suggested change Beta
if ci is True:
if ci:
@michaelkaye

This comment has been minimized.

Copy link

michaelkaye commented Dec 19, 2018

Further to the specific elif comments here #49665 (comment)

I think we need to be quite careful about the API here. Consider this use of the module:

- name: Optionally install the application "coffee-script".
  npm:
    name: coffee-script
    ci: true
    state: "{{ enable_coffee_script | ternary ("present", "absent") }}

I would expect this to install using npm ci if enable_coffee_script, and npm uninstall if not enable_coffee_script.

Current code would run npm ci then run npm uninstall, and adding the elif would make it just run npm ci which is very much /not/ expected.

Can we turn this flag into swapping "npm install" or "npm update" into "npm ci", and not affect whether we install or not?

@ansibot ansibot added stale_ci and removed needs_triage labels Dec 19, 2018

@Bramzor

This comment has been minimized.

Copy link

Bramzor commented Dec 19, 2018

There are a lot of confusing comments so let me be clear. npm ci is NOT like npm install. The difference is that with npm install you can install specific packages while npm ci will actually remove node_modules folder and install all dependancies in one go. So it should not be used to install a specific package or make sure that package does exist like in your example @michaelkaye

Bramzor added some commits Dec 19, 2018

Reverted last commit
npm ci will remove node_modules so cannot be used it to install a specific module.

@ansibot ansibot removed the stale_ci label Dec 19, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 19, 2018

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/packaging/language/npm.py:283:18: trailing-whitespace Trailing whitespace
lib/ansible/modules/packaging/language/npm.py:284:24: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/packaging/language/npm.py:283:19: W291 trailing whitespace
lib/ansible/modules/packaging/language/npm.py:284:25: W291 trailing whitespace

click here for bot help

@michaelkaye

This comment has been minimized.

Copy link

michaelkaye commented Dec 19, 2018

Oh, i see - it's not really a replacement for npm install, it's a separate command that doesn't take many of the options that the npm ansible module can use. I think I must have read the code and saw that the parameters from the module were being passed straight through to npm ci and assumed the command was much more like npm install - apologies for the confusion.

Do you think an extra example like:

- name: Install packages based on package-lock.json.
  npm:
    path: /app/location
    ci: true

Would help make it clear how to use 'ci' (so without including state, name, version etc)

@Bramzor

This comment has been minimized.

Copy link

Bramzor commented Dec 19, 2018

Yes that is how I use it. just path and ci

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Dec 20, 2018

@Bramzor

  1. Do you think it would be useful to add #49665 (comment) as an EXAMPLE?

  2. Could you remove the trailing whitespace

lib/ansible/modules/packaging/language/npm.py:283:19: W291 trailing whitespace
lib/ansible/modules/packaging/language/npm.py:284:25: W291 trailing whitespace
  1. Could you please add a changelog/fragment file for this feature
    https://docs.ansible.com/ansible/devel/community/development_process.html#making-your-pr-merge-worthy

Then we can get this merged into devel for release in Ansible 2.8

@gundalow gundalow self-assigned this Dec 20, 2018

@ansibot ansibot added the stale_ci label Dec 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment