-
Notifications
You must be signed in to change notification settings - Fork 3
specific dockerfiles for each image we may deploy #47
specific dockerfiles for each image we may deploy #47
Conversation
Hey MHBauer! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
hopefully this resolves #38 I can split this up if that's wanted. What we really need is to make sure that each image for both geth nodes and the broker are fully working. |
- broker image has node:alpine and js libs - geth-node uses upstream client-go image with our customizations - Makefile to build images
images/Dockerfile.broker
Outdated
expose 3333 | ||
RUN npm install --production --quiet cli-flags |
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.
LGTM. however testing pusher.js
recently, it appears that cli-flags is not maintained anymore and may not work so we may have to switch to a different node flags library.
If this is the case, we are going to have problem with the ruby version of the broker too.
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.
turns out my problem was with the version of node install on my machine. I was running the very old version 4.6.2
. upgrading to 0.10.10
fixed the initial error I was getting.
That said, can we lock down the version of our binaries.
node: >0.9
ideally 0.10.10
or the latest that works
geth: 1.8.2
worked for me in the past but newer versions could be available.
what other binaries do we have to lock down?
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.
The docker image here is node 10.
I did not get a response earlier when I asked if we needed a specific version of node.
I am not familiar with how node does versions to know what to do.
images/Dockerfile.broker
Outdated
expose 3333 | ||
RUN npm install --production --quiet cli-flags | ||
RUN npm install --production --quiet web3 | ||
RUN npm install --production --quiet solc |
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.
using solc.js
for the time being is alright. We still need to verify that it works with contracts that require input args though. Maybe not a big deal but just for us to keep in mind.
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.
We can get this in and then you can rebase bind on it to have it to play with, or I can take it out and you can determine what's necessary for bind and make this process available to build on.
@@ -5,9 +5,6 @@ language: go | |||
services: | |||
- 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.
how are we going to run unit / integration tests that have dependencies on node?
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.
I think that is issue #36 if you want everything self-contained.
Right now, all docker commands are available in the travis environment, so if we build the image, we can also use the image. Binary should be able to run and talk to the docker daemon.
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 make sure we lock the version of libraries we use.
- node
>= 0.10.10
- geth
>= 1.8.21
- cli-flags node module
2.0.7
- web3.js
1.0.0-beta-36
- solc.js
0.4.25
library versions specified as cli-flags node module 2.0.7 web3.js 1.0.0-beta-36 solc.js 0.4.25
versions updated, |
@nimakaviani everything is set and building? Can we merge? |
sure looks good. Ideally we would like to tag the images we build in the Makefile as I created a repo for |
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.
LGTM
broker image has node:alpine and js libs
geth-node uses upstream client-go image with our customizations
document your feature (added to readme)
comment your code
update the README / how-to-run
have tests (make changes are automatically run by travis)
get reviews in slack