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

use 'npm ci' instead of 'npm install' for frontend builds #48

Closed
dianekaplan opened this issue Dec 23, 2021 · 7 comments
Closed

use 'npm ci' instead of 'npm install' for frontend builds #48

dianekaplan opened this issue Dec 23, 2021 · 7 comments
Assignees
Labels
bug Report of or fix for something that isn't working as intended

Comments

@dianekaplan
Copy link

The gocd generated pipelines use 'npm install' rather than 'npm ci', thanks to tubular/scripts/frontend_build.py and frontend_utils- see here: https://github.com/edx/tubular/blob/master/tubular/scripts/frontend_utils.py#L59

'npm ci' is preferable, as it is more strict about reproducing the package versions set in the package-lock.json (whereas 'npm install' blows it away and can use different versions).

Phil Shiu and David Joy have discussed that this should be corrected to npm ci. Logging here for tracking/info, since it affects many MFEs.

@adamstankiewicz
Copy link
Member

It may be worth prioritizing the npm install -> npm ci change in tubular at the linked shared above over making the change in CI for each individual MFE (though both are important).

Rationale being if we only change the CI in individual MFEs to run npm ci instead, when a PR merges to master in that MFE and runs through the GoCD build and deploy pipeline, it'll still run npm install before releasing to stage/prod, potentially/theoretically pulling in later node_modules than explicitly defined in the package-lock.json file.

I think it may make sense to tackle tubular first and then the individual MFEs.

@abdullahwaheed
Copy link

we are overriding(e.g.) header and footer packages with NPM aliases using edx-internal configuration. Since package-lock.json and package.json will not match with this override, when we'll use npm ci, it would crash!

Ref: https://stackoverflow.com/questions/52499617/what-is-the-difference-between-npm-install-and-npm-ci#answer-53325242

@adamstankiewicz
Copy link
Member

@abdullahwaheed I believe only the npm install for the repo's base dependencies should need to be flipped to npm ci (source), whereas the installation of the NPM alias override(s) would stick to using npm install.

The latter install for NPM alias overrides does not read from package.json or package-lock.json; it just installs over whatever default package was installed in package-lock.json for the repo.

Replicating the build process in GoCD locally works without error, e.g.:

npm ci
npm install @edx/brand@npm:@edx/brand-edx.org@2

Technically, that latter install will modify the package-lock.json file, but those changes are local to GoCD during the build + deploy process and never committed back to the repo. We could prevent the latter npm install for the aliases from mutating package-lock.json by including the --no-save argument.

@abdullahwaheed
Copy link

@adamstankiewicz thank you for clarification. You are right, i have missed the part where we are using package installation and alias installation separately.

@arbrandes
Copy link

What's the current status of this one?

@abdullahwaheed
Copy link

Its PR is ready for review

@arbrandes
Copy link

Looks like tubular is on its way out from the openedx org. Plus, the PR is merged. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
Status: Closed
Development

No branches or pull requests

5 participants