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

Dockerize signsfive-api #22

Merged
merged 12 commits into from
Feb 6, 2018
Merged

Dockerize signsfive-api #22

merged 12 commits into from
Feb 6, 2018

Conversation

meltedspork
Copy link
Contributor

@meltedspork meltedspork commented Feb 4, 2018

This is partly working. But opened PR for purpose of review. .env aren't currently in used since I want to reduce complicated of changes as much as possible.

@meltedspork meltedspork self-assigned this Feb 4, 2018
@meltedspork meltedspork changed the title [WIP] Dockerize sIgnsfive-api [WIP] Dockerize signsfive-api Feb 4, 2018
Copy link
Contributor

@kratsg kratsg left a comment

Choose a reason for hiding this comment

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

Overall, most of the changes look relatively straightforward...

- ./node_modules:/home/node/app/node_modules
- ./scripts:/home/node/app/scripts
- ./src:/home/node/app/src
- ./.npm:/home/node/.npm
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a .npm volume? it's in the .gitignore

Copy link
Contributor Author

@meltedspork meltedspork Feb 4, 2018

Choose a reason for hiding this comment

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

Not necessary but if want able to view the log, this is easiest way unless you want to use docker exec bash

UPDATE: I just realized. We will want that to only happen in during development mode not production. I will make edit on docker-compose.yml and docker-compose.dev.yml 👍

- ./package-lock.json:/home/node/app/package-lock.json
- ./server.log:/home/node/app/server.log
ports:
- 8999:8282
Copy link
Contributor

Choose a reason for hiding this comment

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

how did you decide on the ports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now in .env and only applies to development mode.

command: npm start
networks:
signsfive_net:
ipv4_address: 172.28.1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

how did you decide on ip address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now in .env and only applies to development mode.

depends_on:
- database
ports:
- 9992:80
Copy link
Contributor

Choose a reason for hiding this comment

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

how did you decide on port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now in .env and only applies to development mode.

@meltedspork
Copy link
Contributor Author

I will update documentation about how running in docker and what to do with .env on next commit.

@meltedspork meltedspork changed the title [WIP] Dockerize signsfive-api Dockerize signsfive-api Feb 5, 2018
@meltedspork
Copy link
Contributor Author

This is now out of WIP and PTAL!

README.rst Outdated
@@ -26,6 +26,8 @@ Next, both ``docker-compose.yml`` and ``docker-compose.dev.yml`` are required, s

npm run dev

NOTE: This might take signsfive_api to restart couple of time due to race condition with database starting up.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/issues/26 will try to resolve this race conditions

@kratsg kratsg mentioned this pull request Feb 5, 2018
@meltedspork
Copy link
Contributor Author

Issue has been open to deal with Docker's race condtions - /issues/26

@meltedspork meltedspork mentioned this pull request Feb 6, 2018
@meltedspork
Copy link
Contributor Author

meltedspork commented Feb 6, 2018

I think this is good to be merged in? I made issue to deal with race condtions. Unless it will really break heroku? @kratsg

@meltedspork meltedspork merged commit 64d5bc9 into develop Feb 6, 2018
@kratsg kratsg deleted the feature/dockerize branch February 6, 2018 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants