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

@@ -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.

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

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
     ))