add x86 version by Dashlorde · Pull Request #3 · ole-vi/bellapps

Dashlorde · 2018-02-12T04:39:39Z

No description provided.

empeje · 2018-02-12T06:01:37Z

empeje

Seems good, could you create README.md and a demo?

empeje · 2018-02-12T13:09:19Z

x86/bell/dockerfile

@@ -0,0 +1,25 @@
+FROM node:6.11-alpine
+
+LABEL MAINTAINER Yunlu Zhou <zhouyunlu0216@ole.org>


Add me and @dogi for the future support

empeje · 2018-02-13T05:16:10Z

Will test it today and merge it if its good, again congrats @Dashlorde

empeje

Just realize that there are some unnecessary things there, will review your code again

empeje · 2018-02-13T12:57:56Z

x86/docker-compose.yml

+    image: treehouses/bell:node-latest
+    ports:
+      - "80:80"
+    environment:


I don't think all these environments necessary

empeje · 2018-03-09T16:10:08Z

Related to #1

empeje

Reduce hardcoded stuffs.

empeje · 2018-03-09T16:23:13Z

readme.md

+* Bell. It is bell app container build with node.js.
+* Couchdb. It is the database container which is initiated with `db-init`.
+
+## How to use


I just create https://hub.docker.com/r/treehouses/bell/ and we can actually push the prebuilt image to there.

The readme should contain how to built it manually and how to use the ready-to-go image.

empeje · 2018-03-09T16:23:47Z

x86/bell/dockerfile

+WORKDIR /app/server/src
+
+COPY server.js ./
+COPY package.json ./


why not use original .package.json?

empeje · 2018-03-09T16:24:45Z

x86/bell/dockerfile

+
+ENV version '0.13.21'
+
+RUN mkdir -p /app/server/src


No need to do mkdir -p /app/server/src since it will create new layer. Just do the WORKDIR here.

empeje · 2018-03-09T16:25:52Z

x86/bell/server.js

+
+PortJack.get(/^(.+)$/, function(req, res) {
+    var options = {
+        "127.0.0.1": "http://127.0.0.1:2200/apps/_design/bell/MyApp/index.html",


The address should set by dynamically and specified by the environment variable.

empeje · 2018-03-09T16:26:21Z

x86/bell/server.js

+    if (options.hasOwnProperty(req.hostname)) {
+        res.setHeader('Location', options[req.hostname])
+    } else {
+        res.setHeader('Location', 'http://docker.ole.org:2200/apps/_design/bell/MyApp/index.html')


The address should set by dynamically and specified by the environment variable.

empeje · 2018-03-13T15:40:48Z

Can you create a screenshot of the working bell docker on your machine?
I have limited time to test your setup, but your demonstration will help the review process.

Dashlorde · 2018-03-13T19:34:40Z

steps to set up on local machine:

/bin/bash build.sh to build images
docker-compose up to run container

The address is localhost or 127.0.0.1 which is specified in docker-compose.yml file

empeje · 2018-03-14T09:04:27Z

Awesome @Dashlorde

dogi

@Dashlorde good job - some change wishes ;)

dogi · 2018-03-14T15:55:30Z

readme.md

@@ -0,0 +1,62 @@
+# Bell


readme.md should be README.md

dogi · 2018-03-14T15:56:40Z

x86/bell/dockerfile

+    wget -O BeLL-Apps-${version}.tar.gz \
+    https://github.com/open-learning-exchange/BeLL-Apps/archive/${version}.tar.gz && \
+    tar xvf BeLL-Apps-${version}.tar.gz && \
+    mv BeLL-Apps-${version}/* /app/server/src/


... until here

dogi · 2018-03-14T15:57:09Z

x86/bell/server.js

+
+PortJack.get(/^(.+)$/, function(req, res) {
+
+    res.setHeader('Location', 'http://' + HOST + ':2200/apps/_design/bell/MyApp/index.html')


please use standard port 5984

dogi · 2018-03-14T15:57:55Z

x86/db-init/couchdb-setup.sh

+fi
+
+# Default port for CouchDB accessed from host machine is 2200
+PORT=${PORT:-2200}


please use standard port 5984 and not 2200

dogi · 2018-03-14T15:58:32Z

x86/bell/dockerfile

+
+RUN mkdir -p /app/server/src && \
+    apk add --update openssl && npm install express && \
+    wget -O BeLL-Apps-${version}.tar.gz \


not needed ...

dogi · 2018-03-14T16:00:07Z

x86/db-init/dockerfile

@@ -0,0 +1,24 @@
+FROM node:6.11-alpine


dockerfile should be Dockerfile

dogi · 2018-03-14T16:03:25Z

x86/docker-compose-ready.yml

+      - 5984
+    image: klaemo/couchdb
+    ports:
+      - "2200:5984"


no 2200 please just use 5984

dogi · 2018-03-14T16:03:36Z

x86/docker-compose-ready.yml

+    image: klaemo/couchdb
+    ports:
+      - "2200:5984"
+      - "2201:5986"


dogi · 2018-03-14T16:08:10Z

x86/docker-compose.yml

+      - 5984
+    image: klaemo/couchdb
+    ports:
+      - "2200:5984"


please use 5984 not 2200

dogi · 2018-03-14T16:08:42Z

x86/docker-compose.yml

+    image: klaemo/couchdb
+    ports:
+      - "2200:5984"
+      - "2201:5986"


not needed with klaemo

add x86 version

598f592

empeje reviewed Feb 12, 2018

View reviewed changes

Dashlorde added 3 commits February 12, 2018 23:56

add readme

68728db

update readme

3b3c998

update readme

1057a8b

empeje suggested changes Feb 13, 2018

View reviewed changes

Dashlorde added 2 commits February 13, 2018 14:19

delete environment

dc2c5f4

update remote host name

55108e2

empeje suggested changes Mar 9, 2018

View reviewed changes

Dashlorde added 3 commits March 12, 2018 22:54

change to use environment variable

d4e1282

delete useless server.js

bc8aad1

update readme

3a09592

Dashlorde added 2 commits March 13, 2018 15:28

Reduce hardcoded stuffs

cd78183

fix revert conflit

d56e129

dogi requested changes Mar 14, 2018

View reviewed changes

Dashlorde added 3 commits March 14, 2018 12:56

update to use default port 5984

8259c37

update filename

bcc923f

Delete readme.md

d5654e7

		@@ -0,0 +1,25 @@
		FROM node:6.11-alpine

		LABEL MAINTAINER Yunlu Zhou <zhouyunlu0216@ole.org>


		PortJack.get(/^(.+)$/, function(req, res) {

		res.setHeader('Location', 'http://' + HOST + ':2200/apps/_design/bell/MyApp/index.html')

Conversation

Dashlorde commented Feb 12, 2018

Uh oh!

empeje commented Feb 12, 2018

Uh oh!

empeje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empeje commented Feb 13, 2018

Uh oh!

empeje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empeje commented Mar 9, 2018

Uh oh!

empeje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empeje commented Mar 13, 2018

Uh oh!

Dashlorde commented Mar 13, 2018

Uh oh!

empeje commented Mar 14, 2018

Uh oh!

dogi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants