Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Add Dockerfile and Dockertag generation from templates #12

Closed
wants to merge 22 commits into from

Conversation

iesahin
Copy link
Contributor

@iesahin iesahin commented May 13, 2021

This PR is a major update for the Docker image production.

  • Divided build-all.bash into two (build-all.bash and push-all.bash) and added generate-all.bash that generates Dockerfiles from Dockerfile.template files.

  • Instead of static Dockerfile and Dockertags, the script first generates a set of Dockerfile and Dockertags by calling generate.* scripts in directories.

  • generate.* scripts can be written in bash, sh, zsh, or python. generate-all.bash checks the extension of generate.EXT script and calls the appropriate interpreter.

  • RELEASE_HASH and TAG_PREFIX can be set before calling the script now. Hence running

$ TAG_PREFIX=my_docker_username ./generate-all.bash

produces a set of Dockertag files using my_docker_username/ Docker handle.

DVC Get Started Images

These images are produced by start/generate.bash

For each Git tag in dvc-get-started, a Dockerfile-Dockertag pair is generated in start/build/. Dockerfiles are all depend to python:3.8 public Docker image. Dockertag files use ${TAG_PREFIX}/doc-start:${GIT_TAG} format. For prefix dvcorg, and Git tag signal-file, the produced Docker image is tagged as dvcorg/doc-start:signal-file

Katacoda Image changes

Formerly Katacoda images were produced from static files. In this version, these are also converted to Dockerfile.template files and generated using TAG_PREFIX variable. In the previous version, Dockerfiles had FROM dvcorg/doc-katacoda:base lines. In this version, these are replaced by FROM ${TAG_PREFIX}/doc-katacoda:base, so when the user changes ${TAG_PREFIX}, they also change dependencies among Dockerfiles. This allows for third parties to use these images in their own Dockerhub (or other repository) accounts.

Closes #7
Closes #11
Related iterative/example-repos-dev#34

build-all.bash Outdated

RELEASE_HASH=$(curl -s https://api.github.com/repos/iterative/dvc/releases/latest | sha256sum | cut -c -8)
./generate-all.bash

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why create a separate generate-all.bash yet not use it anywhere else? Either keep things in one file or re-use them in multiple places, surely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this call to generate-all.bash. For debugging and security, generate, build, push should be called separately IMO.

build-all.bash Outdated Show resolved Hide resolved
generate-all.bash Outdated Show resolved Hide resolved
generate-all.bash Outdated Show resolved Hide resolved
generate-all.bash Show resolved Hide resolved
katacoda/generate.bash Outdated Show resolved Hide resolved

ARG RELEASE_HASH

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove? (also same with all other similar cases...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because these are set by environment variables now, with envsubst in generate.bash. Previously RELEASE_HASH was supplied as --build-arg in docker build. ARG in Dockerfile is misleading.

push-all.bash Outdated Show resolved Hide resolved
start/generate.bash Outdated Show resolved Hide resolved
start/generate.bash Outdated Show resolved Hide resolved
build-all.bash Outdated Show resolved Hide resolved
@shcheklein
Copy link
Member

LGTM in general, I didn't check the code behind it though. I hope @casperdcl did this :)

@shcheklein
Copy link
Member

Why does CI fail?

@casperdcl
Copy link

LGTM in general, I didn't check the code behind it though. I hope @casperdcl did this :)

will need a second pass - I suggested obvious fixes in case that helps with CI

@iesahin
Copy link
Contributor Author

iesahin commented May 14, 2021

CI fails because of missing Docker username/password in PR. I removed run in PR from GA. I'll test it afterward.

@iesahin
Copy link
Contributor Author

iesahin commented May 14, 2021

It looks the scripts are running as expected. I run

rm katacoda/build start/build -rf && TAG_PREFIX=emresult ./generate-all.bash && TAG_PREFIX=emresult ./build-all.bash && TAG_PREFIX=emresult ./push-all.bash

and all images are generated and pushed.

e.g. https://hub.docker.com/repository/registry-1.docker.io/emresult/doc-start/tags?page=1&ordering=last_updated

I'll run these containers, test a few commands if they work and push containers to dvcorg.

@iesahin
Copy link
Contributor Author

iesahin commented May 14, 2021

I'll add example-get-started and dvc-checkpoints-mnist images as well. But let's push these first.

@iesahin iesahin requested a review from casperdcl May 15, 2021 15:12
Copy link

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use the GH interface (https://github.com/iterative/dvc-doc-containers/pull/12/files) to respond to reviews (including e.g. "add suggestion to batch" functionality to commit changes)

Comment on lines +10 to +11
# pull_request:
# branches: [ master ]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: revert

Comment on lines +39 to +41
-
name: Generate containers from templates
run: ./generate-all.bash

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appropriately named one-line run command makes name meaningless (same for the cases below)

Suggested change
-
name: Generate containers from templates
run: ./generate-all.bash
- run: ./generate-all.bash

Comment on lines +10 to +15
dockerdir=$(dirname "${filepath}")
tagfile="${dockerdir}/Dockertag"
if [ -f "${tagfile}" ] ; then
TAG=$(head -n 1 "${tagfile}")
else
TAG=$(echo "${dockerdir:3:100}" | tr '/ ' '--')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dockerdir=$(dirname "${filepath}")
tagfile="${dockerdir}/Dockertag"
if [ -f "${tagfile}" ] ; then
TAG=$(head -n 1 "${tagfile}")
else
TAG=$(echo "${dockerdir:3:100}" | tr '/ ' '--')
dockerdir="$(dirname "${filepath}")"
tagfile="${dockerdir}/Dockertag"
if [ -f "${tagfile}" ] ; then
TAG="$(head -n 1 "${tagfile}")"
else
TAG="$(echo "${dockerdir:3:100}" | tr '/ ' '--')"

HERE="$( cd "$(dirname "$0")" ; pwd -P )"
export TAG_PREFIX="${TAG_PREFIX:-dvcorg}"

find . -name generate.* | sort | while read -r filepath ; do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
find . -name generate.* | sort | while read -r filepath ; do
find . -name 'generate.*' | sort | while read -r filepath ; do

Comment on lines +9 to +12
filename=$(basename -- "$filepath")
dirname=$(dirname -- "$filepath")
ext="${filename##*.}"
pushd $dirname

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
filename=$(basename -- "$filepath")
dirname=$(dirname -- "$filepath")
ext="${filename##*.}"
pushd $dirname
filename="$(basename -- "$filepath")"
dirname="$(dirname -- "$filepath")"
ext="${filename##*.}"
pushd "$dirname"

Comment on lines +15 to +22
find ${HERE} -name Dockerfile.template | sort | while read -r filepath ; do
ABSOLUTE_SOURCE=$(dirname "$filepath")
SOURCE_DIR=$(realpath --relative-to="${HERE}" "${ABSOLUTE_SOURCE}")
TARGET_DIR="${HERE}/build/${SOURCE_DIR}"
if [ ! -d "${TARGET_DIR}" ] ; then
mkdir -p "${TARGET_DIR}"
fi
cp -fr ${ABSOLUTE_SOURCE}/* "${TARGET_DIR}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
find ${HERE} -name Dockerfile.template | sort | while read -r filepath ; do
ABSOLUTE_SOURCE=$(dirname "$filepath")
SOURCE_DIR=$(realpath --relative-to="${HERE}" "${ABSOLUTE_SOURCE}")
TARGET_DIR="${HERE}/build/${SOURCE_DIR}"
if [ ! -d "${TARGET_DIR}" ] ; then
mkdir -p "${TARGET_DIR}"
fi
cp -fr ${ABSOLUTE_SOURCE}/* "${TARGET_DIR}"
find "${HERE}" -name Dockerfile.template | sort | while read -r filepath ; do
ABSOLUTE_SOURCE="$(dirname "$filepath")"
SOURCE_DIR="$(realpath --relative-to="${HERE}" "${ABSOLUTE_SOURCE}")"
TARGET_DIR="${HERE}/build/${SOURCE_DIR}"
if [ ! -d "${TARGET_DIR}" ] ; then
mkdir -p "${TARGET_DIR}"
fi
cp -fr "${ABSOLUTE_SOURCE}"/* "${TARGET_DIR}"

Comment on lines +9 to +14
dockerdir=$(dirname "${filepath}")
tagfile="${dockerdir}/Dockertag"
if [ -f "${tagfile}" ] ; then
TAG=$(head -n 1 "${tagfile}")
else
TAG=$(echo "${dockerdir:3:100}" | tr '/ ' '--')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dockerdir=$(dirname "${filepath}")
tagfile="${dockerdir}/Dockertag"
if [ -f "${tagfile}" ] ; then
TAG=$(head -n 1 "${tagfile}")
else
TAG=$(echo "${dockerdir:3:100}" | tr '/ ' '--')
dockerdir="$(dirname "${filepath}")"
tagfile="${dockerdir}/Dockertag"
if [ -f "${tagfile}" ] ; then
TAG="$(head -n 1 "${tagfile}")"
else
TAG="$(echo "${dockerdir:3:100}" | tr '/ ' '--')"

@@ -1,28 +1,25 @@
FROM ubuntu:20.04
FROM python:3.8

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • does the katacode base have to be changed as well?
  • should this be slim?
Suggested change
FROM python:3.8
FROM python:3.8-slim


if [[ -d build ]] ; then
echo "Please delete ${PWD}/build directory first."
exit

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exit
exit 1


export GIT_TAG="${TAG}"

git clone build/dvc-get-started -b "${GIT_TAG}" "${TAG_DIR}/dvc-get-started"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git clone build/dvc-get-started -b "${GIT_TAG}" "${TAG_DIR}/dvc-get-started"
git clone "file://$PWD/build/dvc-get-started/.git" -b "${GIT_TAG}" "${TAG_DIR}/dvc-get-started"

@iesahin iesahin closed this Nov 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants