Skip to content

do a production yarn install in the build step#5391

Merged
mitchellhenke merged 5 commits intomainfrom
mitchellhenke/yarn-install
Sep 10, 2021
Merged

do a production yarn install in the build step#5391
mitchellhenke merged 5 commits intomainfrom
mitchellhenke/yarn-install

Conversation

@mitchellhenke
Copy link
Contributor

Ignores dev dependencies and ensures the lock file is not modified or out of sync

Deployed this change to my sandbox and the install and asset compilation still work as expected

@mitchellhenke mitchellhenke requested a review from aduth September 9, 2021 19:11
@aduth
Copy link
Contributor

aduth commented Sep 9, 2021

Also tentatively considered in #4696. Looking at the dependencies split, it seems we should be fine, though moving forward we'll need to be careful to always include build dependencies as dependencies, not devDependencies. Per #4696, I wonder if there might be a way to test for this by doing yarn install --prod for the "build" CircleCI job?

@aduth
Copy link
Contributor

aduth commented Sep 9, 2021

I wasn't aware of --frozen-lockfile. I wonder if we could use that as a substitute for what we use make lint_yarn_lockfile for?

identity-idp/Makefile

Lines 58 to 59 in 905e257

lint_yarn_lockfile:
(! git diff --name-only | grep yarn.lock) || (echo "Error: Sync Yarn lockfile using 'yarn install'"; exit 1)

@mitchellhenke
Copy link
Contributor Author

mitchellhenke commented Sep 9, 2021

Per #4696, I wonder if there might be a way to test for this by doing yarn install --prod for the "build" CircleCI job?

I wasn't aware of --frozen-lockfile. I wonder if we could use that as a substitute for what we use make lint_yarn_lockfile for?

identity-idp/Makefile

Lines 58 to 59 in 905e257

lint_yarn_lockfile:
(! git diff --name-only | grep yarn.lock) || (echo "Error: Sync Yarn lockfile using 'yarn install'"; exit 1)

On both of those, we may be able to do the same yarn install --production --frozen-lockfile in CircleCI since in theory we shouldn't need devDependencies in CI either, and if something breaks, we can catch it there?

I am pushing a test of it in 864fc2cf6791dab525cdb7c87bc17bc762536dea

If it does work we can drop the lint_yarn_lockfile too

@aduth
Copy link
Contributor

aduth commented Sep 9, 2021

We'll need development dependencies for some of the CircleCI jobs like linting / JavaScript tests.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/yarn-install branch from 864fc2c to c974bc4 Compare September 9, 2021 20:25
@mitchellhenke
Copy link
Contributor Author

Oops, yep, dropping the --production in CI

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/yarn-install branch from c974bc4 to df099bd Compare September 9, 2021 20:27
@aduth
Copy link
Contributor

aduth commented Sep 9, 2021

I guess my ideal would be where the "build" job specifically could install only production dependencies, while "lints" and "javascript_build" continue to install all dependencies, so that "build" acts most similar to a runtime environment. Implementing this seems challenging though 😬

@mitchellhenke
Copy link
Contributor Author

mitchellhenke commented Sep 9, 2021

I guess my ideal would be where the "build" job specifically could install only production dependencies, while "lints" and "javascript_build" continue to install all dependencies, so that "build" acts most similar to a runtime environment. Implementing this seems challenging though 😬

Required some splitting up of the install parts and a little more verbosity, but it's not TOO bad in e9fe6e84e?

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/yarn-install branch from f027afb to e9fe6e8 Compare September 9, 2021 20:40
@aduth
Copy link
Contributor

aduth commented Sep 10, 2021

Looks like the build got stuck?

Also, any thoughts whether javascript_build should be caching or at least using (restoring) cache for dependencies? Or maybe not, if it'd not be a bottleneck since it should otherwise be relatively fast?

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/yarn-install branch from e9fe6e8 to ab78c2b Compare September 10, 2021 16:08
@mitchellhenke
Copy link
Contributor Author

Also, any thoughts whether javascript_build should be caching or at least using (restoring) cache for dependencies? Or maybe not, if it'd not be a bottleneck since it should otherwise be relatively fast?

I went back and forth on it a bit, but the effort to do it is relatively low, so we may as well do it while we're here even if it's not a bottleneck right now. Added a separate cache for the --production install in 4874e2a

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

command: |
yarn test

javascript_build:
Copy link
Contributor

@aduth aduth Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the name here as far as verifying that we can actually build assets, but it leaves me feeling a bit funny that we have the unprefixed "build" job as meaning something totally different, where it's more about running RSpec tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, changed the build job to be named ruby_test

@mitchellhenke mitchellhenke merged commit 73bc63d into main Sep 10, 2021
@mitchellhenke mitchellhenke deleted the mitchellhenke/yarn-install branch September 10, 2021 16:55
@aduth
Copy link
Contributor

aduth commented Dec 6, 2021

Hm, I just ran into a case where yarn install --frozen-lockfile had no issues, but yarn install introduced new changes to yarn.lock (86f7df4) 😕 Maybe that flag isn't quite as strict as we'd hope.

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.

2 participants