From 437418d36757384e0865d44a860fe22302b7aa5d Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Thu, 29 Jun 2023 17:55:23 -0400 Subject: [PATCH] feat: Switch Dockerfile from `npm install` to `npm ci`; some cleanup (#32590) Otherwise we're not really respecting the package-lock file and won't get repeatable results. Also: - Clean up old error handling for npm<3. Were on npm 8 now. Probably can get rid of this. - Use the shorthand `npm ci` rather than `npm clean-install` just for consistency with code elsewhere. - Update comments in tests to be explicit about use of ci rather than install --- Dockerfile | 2 +- pavelib/paver_tests/test_prereqs.py | 10 +++++----- pavelib/prereqs.py | 11 ++--------- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/Dockerfile b/Dockerfile index 94bea46e56..6948fefcf8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -119,7 +119,7 @@ RUN nodeenv /edx/app/edxapp/nodeenv --node=16.14.0 --prebuilt RUN npm install -g npm@8.5.x COPY package.json package.json COPY package-lock.json package-lock.json -RUN npm set progress=false && npm install +RUN npm set progress=false && npm ci # The builder-development stage is a temporary stage that installs python modules required for development purposes # The built artifacts from this stage are then copied to the development stage. diff --git a/pavelib/paver_tests/test_prereqs.py b/pavelib/paver_tests/test_prereqs.py index 2d4bdd6ab9..4b542ae97b 100644 --- a/pavelib/paver_tests/test_prereqs.py +++ b/pavelib/paver_tests/test_prereqs.py @@ -87,7 +87,7 @@ class TestPaverNodeInstall(PaverTestCase): def test_npm_install_with_subprocess_error(self): """ - Test an error in 'npm install' execution + Test an error in 'npm ci' execution """ with patch('subprocess.Popen') as _mock_popen: _mock_subprocess = mock.Mock() @@ -97,21 +97,21 @@ class TestPaverNodeInstall(PaverTestCase): with pytest.raises(Exception): pavelib.prereqs.node_prereqs_installation() - # npm install will be called twice + # npm ci will be called twice assert _mock_popen.call_count == 2 def test_npm_install_called_once_when_successful(self): """ - Vanilla npm install should only be calling npm install one time + Vanilla npm ci should only be calling npm ci one time """ with patch('subprocess.Popen') as _mock_popen: pavelib.prereqs.node_prereqs_installation() - # when there's no failure, npm install is only called once + # when there's no failure, npm ci is only called once assert _mock_popen.call_count == 1 def test_npm_install_with_unexpected_subprocess_error(self): """ - If there's some other error, only call npm install once, and raise a failure + If there's some other error, only call npm ci once, and raise a failure """ with patch('subprocess.Popen') as _mock_popen: _mock_popen.side_effect = unexpected_fail_on_npm_install diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py index 33cbab7ef9..ebb49aeacb 100644 --- a/pavelib/prereqs.py +++ b/pavelib/prereqs.py @@ -137,7 +137,7 @@ def node_prereqs_installation(): else: npm_log_file_path = f'{Env.GEN_LOG_DIR}/npm-install.log' npm_log_file = open(npm_log_file_path, 'wb') # lint-amnesty, pylint: disable=consider-using-with - npm_command = 'npm clean-install --verbose'.split() + npm_command = 'npm ci --verbose'.split() # The implementation of Paver's `sh` function returns before the forked # actually returns. Using a Popen object so that we can ensure that @@ -145,14 +145,7 @@ def node_prereqs_installation(): proc = subprocess.Popen(npm_command, stderr=npm_log_file) # lint-amnesty, pylint: disable=consider-using-with retcode = proc.wait() if retcode == 1: - # Error handling around a race condition that produces "cb() never called" error. This - # evinces itself as `cb_error_text` and it ought to disappear when we upgrade - # npm to 3 or higher. TODO: clean this up when we do that. - print("npm clean-install error detected. Retrying...") - proc = subprocess.Popen(npm_command, stderr=npm_log_file) # lint-amnesty, pylint: disable=consider-using-with - retcode = proc.wait() - if retcode == 1: - raise Exception(f"npm install failed: See {npm_log_file_path}") + raise Exception(f"npm install failed: See {npm_log_file_path}") print("Successfully clean-installed NPM packages. Log found at {}".format( npm_log_file_path ))