Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Fix for Dockerfile issues building on CentOs 7. See issue #1081 #1082

Merged
merged 3 commits into from
May 18, 2017

Conversation

RonanOD
Copy link
Contributor

@RonanOD RonanOD commented May 10, 2017

Fixes #1081

Changes proposed in this pull request:

  • Get Dockerfile to build on CentOS 7
  • Remove unnecessary npm commands that weren't working

Hi, I solved my difficulties with building the HospitalRun docker image by making these slight changes to the Dockerfile. Hopefully they are acceptable, if not can you let me know what else I should do to get this working.
Thanks

@tangollama
cc @HospitalRun/core-maintainers

Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

@RonanOD thanks for the PR! I think that the changes you made need some slight tweaking. Please see below for more details.

Dockerfile Outdated
@@ -9,8 +9,9 @@ RUN apt-get update && apt-get install nodejs -y
RUN mkdir -p /usr/src/app
WORKDIR /usr/src/app
COPY . /usr/src/app
RUN npm install -g npm && npm install -g ember-cli@latest && npm install -g bower
RUN npm install
RUN npm install -g ember-cli
Copy link
Member

Choose a reason for hiding this comment

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

I think the only fix needed here is to remove the npm install -g npm.

Dockerfile Outdated
RUN npm install -g npm && npm install -g ember-cli@latest && npm install -g bower
RUN npm install
RUN npm install -g ember-cli
RUN latest && npm install -g bower
Copy link
Member

Choose a reason for hiding this comment

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

The latest should have been on the previous line (eg npm install -g ember-cli@latest)

Dockerfile Outdated
RUN npm install
RUN npm install -g ember-cli
RUN latest && npm install -g bower
#RUN npm install
Copy link
Member

Choose a reason for hiding this comment

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

This line is needed to run HospitalRun

@RonanOD
Copy link
Contributor Author

RonanOD commented May 11, 2017

Thanks @jkleinsc. Made the improvements you suggested and pushed to this branch.

Copy link
Member

@jkleinsc jkleinsc 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 to me. Thanks for the update @RonanOD!

@jkleinsc jkleinsc merged commit 393127e into HospitalRun:master May 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants