Closed Bug 1340476 Opened 7 years ago Closed 7 years ago

Create the two Dockerfiles for the static UI (1 to build it, 1 to contain the files without dependencies).

Categories

(Conduit :: General, defect)

Product:
Conduit
Component:
General
Platform:
x86_64
Windows 10
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: imadueme, Assigned: imadueme)

Details

Attachments

(1 file)

autoland: create docker file to contain the built static assets on circle ci (bug 1340476).
59 bytes, text/x-review-board-request
smacleod
: review+
Details 
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review114856

::: autoland/docker-compose.yml:21
(Diff revision 1)
>        context: ./ui
>        dockerfile: ./Dockerfile-dev
>      command: start
>      volumes:
>        - ./ui/src:/code/src
> +      - ./ui/build:/code/build

This volume mount is needed to connect the ui devcontainer to the non-docker host that circle CI runs on which can then connect it to the new docker container that simpily copies the assets in.

This does not affect UI development in anyway. Actually it is sort of good because now we can inspect the build files that are being produced.

::: autoland/ui/Dockerfile-prod:5
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +FROM node:7-alpine

I know we should change this image since we don't need node, but, what should it be?

::: autoland/ui/Dockerfile-prod:10
(Diff revision 1)
> +FROM node:7-alpine
> +
> +RUN mkdir -p /deploy_assets
> +COPY ./build /deploy_assets
> +
> +CMD ["/bin/bash"]

This is here so that you can do stuff like `docker run autolandui ls /deploy_assets && rsync ...` or whatever.

Do you think it is needed?

::: circle.yml:28
(Diff revision 1)
>      - invoke version > version.json
>      - cp version.json $CIRCLE_ARTIFACTS
>  
> +    # build development images for tests:
> +    - docker-compose -f autoland/docker-compose.yml build autolandweb
> +    - docker-compose -f autoland/docker-compose.yml build yarn

we need to build the ui dev container first so that it can compile the react app into just static assets.

It looks like it already copies over the images folder and stuff like that.

::: circle.yml:36
(Diff revision 1)
>      - cp version.json autoland/public-web-api/
>      - docker build -t autoland.web:build -f autoland/public-web-api/Dockerfile-prod autoland/public-web-api
>      - docker images --no-trunc | awk '/^autoland.web/ {print $3}' | tee $CIRCLE_ARTIFACTS/autoland.web-image-shasum256.txt
>  
> -    # build development images for tests:
> -    - docker-compose -f autoland/docker-compose.yml build autolandweb
> +    # build autolandui:
> +    - docker-compose -f autoland/docker-compose.yml run yarn build

Run the yarn build command to output the `build` directory to `autoland/ui/build` thanks to to volume mount in the docker-compose file.

::: circle.yml:38
(Diff revision 1)
>      - docker images --no-trunc | awk '/^autoland.web/ {print $3}' | tee $CIRCLE_ARTIFACTS/autoland.web-image-shasum256.txt
>  
> -    # build development images for tests:
> -    - docker-compose -f autoland/docker-compose.yml build autolandweb
> -    - docker-compose -f autoland/docker-compose.yml build yarn
> +    # build autolandui:
> +    - docker-compose -f autoland/docker-compose.yml run yarn build
> +    - docker build -t autoland.ui:build -f autoland/ui/Dockerfile-prod autoland/ui
> +    - docker images --no-trunc | awk '/^autoland.ui/ {print $3}' | tee $CIRCLE_ARTIFACTS/autoland.ui-image-shasum256.txt

This could be totally wrong, I just added it cause it seemed ok.

::: circle.yml:55
(Diff revision 1)
>      commands:
>        - "[ ! -z $DOCKERHUB_REPO ]"
>        - docker login -e $DOCKER_EMAIL -u $DOCKER_USER -p $DOCKER_PASS
>        - "docker tag autoland.web:build ${DOCKERHUB_REPO}:autoland.web.${CIRCLE_SHA1}"
>        - "docker push ${DOCKERHUB_REPO}:autoland.web.${CIRCLE_SHA1}"
> +      - "docker tag autoland.ui:build ${DOCKERHUB_REPO}:autoland.ui.${CIRCLE_SHA1}"

This also could be totally wrong, but you get the idea: push the ui image to dockerhub.
Attachment #8838468 - Flags: review-
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review114856

> I know we should change this image since we don't need node, but, what should it be?

I would base the image on busybox.

> This is here so that you can do stuff like `docker run autolandui ls /deploy_assets && rsync ...` or whatever.
> 
> Do you think it is needed?

I'd change it to whatever the command is that actually builds the static assets in one go, if there is such a command.  If there is no command, then I might leave CMD out entirely - 80/20 rule, make CMD do what you need most of the time.
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review115604

::: autoland/docker-compose.yml:13
(Diff revision 2)
>      image: nginx:alpine
>      ports:
>        - "8888:80"
>      volumes:
>        - ./docker/web/nginx-conf.d:/etc/nginx/conf.d:ro
>  

Please take a look at my comments in the last revision :)
Attachment #8838468 - Flags: review-
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review114856

> I would base the image on busybox.

We should probably pick a *much* smaller container without node. Really we're just using a docker image to package files so the base image doesn't need any dependencies at all. Maybe do a search to see if others are doing this and what base image they use.

> I'd change it to whatever the command is that actually builds the static assets in one go, if there is such a command.  If there is no command, then I might leave CMD out entirely - 80/20 rule, make CMD do what you need most of the time.

I think a bunch of base images leave bash out, and just have /bin/sh - maybe we should use that? I don't remember why I think this though, so I could be wrong.

Also, I think we need to have at least something in CMD:

> Dockerfile should specify at least one of CMD or ENTRYPOINT commands

https://docs.docker.com/engine/reference/builder/#/understand-how-cmd-and-entrypoint-interact
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review115616

::: autoland/ui/Dockerfile-prod:7
(Diff revision 2)
> +RUN mkdir -p /deploy_assets
> +COPY ./build /deploy_assets

We should keep things consistent with Dockerflow and just stick the assets in `/app/`, which is where the production build will also stick the version.json.

::: circle.yml:26
(Diff revision 2)
> +    # build development images for tests:
> +    - docker-compose -f autoland/docker-compose.yml build webapi
> +    - docker-compose -f autoland/docker-compose.yml build yarn

Having these before our production images might mess with the awk commands below... hmmm... We might want to consider refactoring things a bit.

::: circle.yml:28
(Diff revision 2)
>      - invoke version > version.json
>      - cp version.json $CIRCLE_ARTIFACTS
>  
> +    # build development images for tests:
> +    - docker-compose -f autoland/docker-compose.yml build webapi
> +    - docker-compose -f autoland/docker-compose.yml build yarn

Do you see the dev container ever growing anything we'd not want to use for our production build? different build settings? etc? if so we might want to go with a distinct image for building production.

::: circle.yml:35
(Diff revision 2)
> -    # build development images for tests:
> -    - docker-compose -f autoland/docker-compose.yml build webapi
> -    - docker-compose -f autoland/docker-compose.yml build yarn
> +    # build autoland/ui:
> +    - docker-compose -f autoland/docker-compose.yml run yarn build
> +    - docker build -t autoland.ui:build -f autoland/ui/Dockerfile-prod autoland/ui
> +    - docker images --no-trunc | awk '/^autoland.ui/ {print $3}' | tee $CIRCLE_ARTIFACTS/autoland.ui-image-shasum256.txt

We're going to need to put the version.json inside the ui image as well.

::: circle.yml:48
(Diff revision 2)
>    autoland_webapi_latest:
>      branch: "master"
>      commands:
>        - "[ ! -z $DOCKERHUB_REPO ]"
>        - docker login -e $DOCKER_EMAIL -u $DOCKER_USER -p $DOCKER_PASS
>        - "docker tag autoland.webapi:build ${DOCKERHUB_REPO}:autoland.webapi.${CIRCLE_SHA1}"
>        - "docker push ${DOCKERHUB_REPO}:autoland.webapi.${CIRCLE_SHA1}"
> +      - "docker tag autoland.ui:build ${DOCKERHUB_REPO}:autoland.ui.${CIRCLE_SHA1}"
> +      - "docker push ${DOCKERHUB_REPO}:autoland.ui.${CIRCLE_SHA1}"

My plan was to have each of the images we push be their own deployment task here "autoland_webapi_latest", "autoland_ui_latest" etc.
Attachment #8838468 - Flags: review?(smacleod) → review-
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review115616

> Having these before our production images might mess with the awk commands below... hmmm... We might want to consider refactoring things a bit.

I don't think they will as they aren't prefixed with autoland...dropping for now and I guess we will find out if you land this :)
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review115616

> I don't think they will as they aren't prefixed with autoland...dropping for now and I guess we will find out if you land this :)

Actually it might, but the difference is an `_` vs a `.` it seems
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review116100
Attachment #8838468 - Flags: review?(smacleod) → review+
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/automation/conduit/rev/5459094fde5b
autoland: create docker file to contain the built static assets on circle ci . r=smacleod
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review115616

> Do you see the dev container ever growing anything we'd not want to use for our production build? different build settings? etc? if so we might want to go with a distinct image for building production.

During our 1on1 we talked about this, I presonally don't see the dev container growing to be more than this. We could possible just rename it to remove the stigma of using a container called dev to build production :)

> My plan was to have each of the images we push be their own deployment task here "autoland_webapi_latest", "autoland_ui_latest" etc.

Sorry I dropped this, was unsure if you wanted me to handle this or if you wanted to.
Comment on attachment 8838468 [details]
autoland: create docker file to contain the built static assets on circle ci (bug 1340476).

https://reviewboard.mozilla.org/r/113408/#review116530
Attachment #8838468 - Flags: review?(mars)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: