Fixed docker build #349
Fixed docker build #349
Conversation
Dockerfile
Outdated
|
|
||
| COPY . /app | ||
|
|
||
| CMD [ "./zenbot.sh", "trade" ] |
There was a problem hiding this comment.
not sure if this one is necessary, as we also rely on it to run sims
There was a problem hiding this comment.
also this command is specified inside docker-compose file so you need to decide which one you want to use to run the command
There was a problem hiding this comment.
Yes, the default command for the bot to run is running ./zenbot trade. However it can be overwritten in docker-compose.yml or when running as a docker run command.
There was a problem hiding this comment.
I feel like it may confuse people, especially those who are new to Docker. Either way you should pass --paper by default so people don't accidentally trigger live trading
There was a problem hiding this comment.
OK. That makes sense 👍 I'll fix it.
There was a problem hiding this comment.
If you want to make these changes I would go different route and create another docker file, one for development and one for production/live trading. Otherwise someone else will create another PR to revert these changes making it more suitable for development.
| RUN npm install | ||
|
|
||
| COPY . /app |
There was a problem hiding this comment.
wouldn't this override node_modules if you install them locally before running docker?
There was a problem hiding this comment.
The .dockerignore file includes node_modules, so it won't be copied
Before the docker CLI sends the context to the docker daemon, it looks for a file named .dockerignore in the root directory of the context. If this file exists, the CLI modifies the context to exclude files and directories that match patterns in it. This helps to avoid unnecessarily sending large or sensitive files and directories to the daemon and potentially adding them to images using ADD or COPY.
There was a problem hiding this comment.
Gotcha, thanks for clarifying I didn't notice someone added .dockerignore
Dockerfile
Outdated
|
|
||
| COPY . /app | ||
|
|
||
| CMD [ "./zenbot.sh", "trade" ] |
There was a problem hiding this comment.
also this command is specified inside docker-compose file so you need to decide which one you want to use to run the command
| @@ -1,8 +1,7 @@ | |||
| server: | |||
| build: . | |||
| volumes: | |||
| - .:/app | |||
| - /app/node_modules | |||
| - ./conf.js:/app/conf.js | |||
There was a problem hiding this comment.
this won't work for development purposes
There was a problem hiding this comment.
No, I agree. For development purposes I would probably mount in more directories. I would even consider running the environment without docker container.
I could also make a docker-compose.development.yml which would override the main docker-compose.yml with extra volume mappings.
There was a problem hiding this comment.
i still think it should be the other way around, have docker-compose.yml as your default dev environment and docker-compose.live.yml as your live trading one. @DeviaVir what do you think?
There was a problem hiding this comment.
@egorbenko I like this lightweight version as the default a bit better, developers will know to use docker-compose.development.yml while your "average" user won't intuitively understand how to use docker-compose.live.yml. Fully understand where you're coming from though.
There was a problem hiding this comment.
now that i think of it maybe docker-compose.development.yml makes more sense, as long as there is a flag for --paper trading by default in both Dockerfile and docker-compose
|
@egorbenko I've made the changes you requested. |
|
Looks good to me, maybe add some documentation in your follow up PR with |
|
Yes - I will @egorbenko . |
…os8f-master * 'master' of https://github.com/carlos8f/zenbot: Improve command help for train command (DeviaVir#436) Support for missing win/loss and error rate from output (DeviaVir#426) Fix docker builds for forex.analytics (DeviaVir#424) Minor fixes for Quadriga trading API (DeviaVir#421) xmpp for trading alarms (DeviaVir#333) Strategies: TA (ema+macd) and Trust/Distrust (DeviaVir#285) Fixed docker build (DeviaVir#349) updated c.default_selector to c.selector in README.md (DeviaVir#418) added check for message before doing anything with it (DeviaVir#412) FIX: Properly check for unknown indicators in forex_analytics (DeviaVir#408) Added support for the BTCe exchange (DeviaVir#388) Add strategy: forex.analytics, an genetic optimization algorithm for TA-lib stats (DeviaVir#389) # Conflicts: # .gitignore # commands/sim.js # extensions/exchanges/bittrex/exchange.js # extensions/exchanges/quadriga/exchange.js # lib/engine.js # scripts/auto_backtester/backtester.js
* Fixed docker build * Added --paper according to github comments
Removed the mapping of the entire map directory: This should not be mapped in runtime, as necessary files actually are copied to the container on build-time.
Mapped conf.js: This file is the only file needed, that is not all-ready in the build container.
Dockerfile restructure: More effecient for development, as dependencies are only fetched if you actually change them.
See: https://nodejs.org/en/docs/guides/nodejs-docker-webapp/