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

#54 Docker update #61

Merged
merged 14 commits into from
Nov 5, 2018
Merged

#54 Docker update #61

merged 14 commits into from
Nov 5, 2018

Conversation

microwavekonijn
Copy link
Collaborator

@microwavekonijn microwavekonijn commented Nov 1, 2018

This update will give us some benefits. Now the Dockerfile is the same for testing and running the bot. Also it makes smart use of the cache when building containers, making rebuild times of containers a lot faster. Also it will auto start the bot when the container starts.

Issue #54

- Bot will automatically run on start container
- Ignore node_modules and local.json when building
- Tests use the regular Dockerfile, given more consistency
- Travis installs the missing dev dependencies
- Bot starts now by calling npm start
- Production and staging compose use now the build image from Docker Hub
- Building the container will only install normal dependencies
- Added comments for clarification
@JamesLongman JamesLongman added this to the v0.1.0 milestone Nov 1, 2018
@JamesLongman JamesLongman self-requested a review November 1, 2018 23:20
@JamesLongman JamesLongman added this to In progress in Infrastructure Update Nov 1, 2018
- npm-debug.log ingnore by npm and docker
- Added scripts to npm for local testing and running using a docker container
- Fixed container naming in docker-compose for production
Copy link
Contributor

@JamesLongman JamesLongman left a comment

Choose a reason for hiding this comment

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

Definately more efficient build order and better use of compose files. The way npm install is split is very efficient.

Needs docs to explain npm run-script x

Our current docs need to be improving not disapearing, docker/README.md can be moved or its information can be put in the main readme but it can't just be deleted.

Usually I would say we need to make sure we're including project file changes in tests but #60 is blocking that so no need

@@ -4,7 +4,10 @@
"description": "- Nodejs 7.x\r - npm: discord.js",
"main": "index.js",
"scripts": {
"test": "mocha"
"start": "node src/bot.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's swap start with start:docker and test with test:docker to make this mildly simpler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to leave it likes this. test is used by travis directly and start is used by the container, I just added start:docker and test:docker to make running those container easier for our own benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not just swap the values in references? I'm just thinking the local version is going to be the one we're typing and I'd rather type start 5000 times than start:docker

Copy link
Collaborator Author

@microwavekonijn microwavekonijn Nov 5, 2018

Choose a reason for hiding this comment

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

You can use up arrow to reuse previous commands you entered. Also in ide's(at least Webstorm) you can add a script which you can run with the press of a button. Honestly I would like to look at a solution(and I know it is possible) where it detects changes and then restarts the bot with the new code.

Besides, instead of building a container each time we make changes(even with use of the docker cache now) we should really look into just running the bot locally without docker. The only problem is that one of the dependencies uses python and needs to compile some stuff (and I have no idea how to fix it). To make sure that we run the bot with the intended node version we can use nvm, tho running it with a newer version should not bring any problems (just it will support newer features, which will fail during testing when implemented).

Edit: Added up and down scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

As the section currently stands if we change test --> mocha and test:docker --> test I think this would be good to merge.

Automatic rebuilding would be cool, but running the bot outside of our docker environment would be very bad for consistency. The whole point of docker setup is to hopefully provide identical environments on all systems. You are of course free to do whatever you want locally but I would recommend against it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Differences between platforms and versions are very minimal as far as I can tell. We should probably set some requirement conditions for which the bot should work with (like the nodejs version). We can always expand it when we find problems, but we don't really want to trade ease of development for avoidance of possible problems we might encounter. Also testing should catch any problems and else be expanded to catch those problems. I already encountered this when I used finally on a promise.

package.json Outdated Show resolved Hide resolved
docker-compose-staging.yml Outdated Show resolved Hide resolved
.dockerignore Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
microwavekonijn and others added 6 commits November 5, 2018 14:54
- .dockerignore is mad at some files
- renamed some script and added some more
- Added some documentation with requirements and instructions
- Added default env vars to the Dockerfile
- The port in the container is now exposed by default to port 1337, this can be overwritten by mapping the port to another port on the host
- npm run up is alias for npm run docker:start && npm run docker:logs
- npm run down is alias for npm run docker:stop
README.md Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@JamesLongman JamesLongman merged commit 334b6c9 into develop Nov 5, 2018
@JamesLongman JamesLongman moved this from In progress to Done in Infrastructure Update Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants