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

[OSF-7362] Use yarn instead of npm to install modules #6746

Merged
merged 2 commits into from
Jan 25, 2017
Merged

[OSF-7362] Use yarn instead of npm to install modules #6746

merged 2 commits into from
Jan 25, 2017

Conversation

binoculars
Copy link
Member

@binoculars binoculars commented Jan 19, 2017

❗️ Depends on CenterForOpenScience/ember-osf-preprints#249

Purpose

Replaces npm with yarn (better node.js dependency management)

Changes

Updates a single line in the docker-compose file

Side effects

N/A

Ticket

https://openscience.atlassian.net/browse/OSF-7362

@brianjgeiger brianjgeiger changed the title [EOSF-447] Use yarn instead of npm to install modules [OSF-7362] Use yarn instead of npm to install modules Jan 19, 2017
@@ -166,7 +166,7 @@ services:

preprints:
image: quay.io/centerforopenscience/ember-preprints:develop
command: /bin/bash -c "npm install && ./node_modules/bower/bin/bower install --allow-root --config.intedractive=false && ./node_modules/ember-cli/bin/ember serve --host 0.0.0.0 --port 4200"
command: /bin/bash -c "yarn --pure-lockfile --no-progress --no-emoji && ./node_modules/bower/bin/bower install --allow-root --config.interactive=false && ./node_modules/ember-cli/bin/ember serve --host 0.0.0.0 --port 4200"
Copy link
Contributor

Choose a reason for hiding this comment

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

Our other Dockerfiles needed to install Yarn. Can you clarify why the OSF does not need this change?

Also: other PRs activating Yarn do not use some of these flags. If we find they are valuable, we might want to use the same consistent commands / set of flags across all of our repos.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a change to the docker-compose file, so It's using the preprints image, but running a different command. This is so that, when developing, the local changes will be synced in the container, and ember serve will be running (which will trigger ember builds as files are changed).

The reason for the extra flags --no-progress and --no-emoji is that console output console output is superfluous here. You want the output during a regular image build (docker built --tag whatever .). Yarn is smart enough to know when it's in a CI environment, so it's not necessary to add these flags in the travis config.

Copy link
Contributor

@abought abought Jan 19, 2017

Choose a reason for hiding this comment

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

Interesting! And yes, I didn't look at the line above closely enough.

So we're not quite at the point of using Yarn for the OSF yet- just making sure that preprints obeys the settings we used elsewhere.

It's interesting that we need this command in addition to the Dockerfile in preprints. Still getting a handle on the interdependencies in different repos. Thanks.

@abought
Copy link
Contributor

abought commented Jan 19, 2017

Yarn has two benefits: fast installation and version locking.

I don't see a yarn.lock file in this repo, however, either before or after this change. Is there a specific reason why one isn't added?

@abought
Copy link
Contributor

abought commented Jan 19, 2017

Confirmed that the container builds and runs with this change. Two questions for developer before moving ahead, but hopefully these will both be straightforward.

@binoculars
Copy link
Member Author

binoculars commented Jan 19, 2017

My objective with this PR was not to add yarn to the main OSF repo, but to make sure that we run yarn commands consistently within the preprints container.

@binoculars
Copy link
Member Author

@icereval @brianjgeiger Need this merged since ember-preprints develop is using yarn

@brianjgeiger
Copy link
Collaborator

@binoculars Is it needed needed? I know we want to move OSF over, but my impression was that the ember apps and OSF not directly linked, and staging right now is running with preprints moved to Yarn but not the OSF. I'm not opposed to doing it in general, but I'd probably have @sloria take a quick look before moving the OSF over.

@binoculars
Copy link
Member Author

This is just so that the build uses yarn when you start the container up in development. The command is overridden for production.

@brianjgeiger brianjgeiger merged commit 0ab10be into CenterForOpenScience:develop Jan 25, 2017
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.

None yet

3 participants