Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#54 Docker update #61
Changes from all commits
2aa895c
e791ef9
dcd5235
9568e3d
ac1850f
a8936b4
d7f154a
1079d96
062e7fd
3e65349
4009e9e
8315cab
b9edbed
a0348cb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's swap
start
withstart:docker
andtest
withtest:docker
to make this mildly simplerThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andtest: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
There was a problem hiding this comment.
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.
This file was deleted.