Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use github actions to build docker images #29

Merged
merged 22 commits into from
Oct 17, 2021
Merged

use github actions to build docker images #29

merged 22 commits into from
Oct 17, 2021

Conversation

sidey79
Copy link
Contributor

@sidey79 sidey79 commented Dec 29, 2020

Build process via github actions

  • Added a workflow which builds the fhem docker image for serveral architectures via buildx supported plattforms
  • Upload for test of the images currently provided to github container registry if there is a secret token provided,

disabled travis CI builds

  • Due to missing travis credits, the travis yaml file is renamed to not force any more builds

Changes in Dockerfile

  • ARG statements are initialized with default params and as late as possible to avoid cache invalidation

  • COPY statements combined to reduce layers

  • RUN statements are sorted to get most caching for different builds

  • Reference version of base dockerimage and also reference apt version to invalidate cache if new version is needed

  • Limited nodejs support for plattforms where it is supported, some more work can be done here using a already existing nodejs image, but may it is better to run this in a separate docker server

  • Usage of ARCH variable has changed, because ARCH itself isn't a common variable from buildx instead TARGETARCH is used

  • Integrationtest script runs against amd64 image

  • build script now uses buildx, buildx is installed if needed

  • Unittests are executed against amd64 image

  • Added dependabot script to detect changed base image

  • Build tow images vairiants using different layer options via separate action step

  • Build now uses the slim image from debian as docker base image

  • Build time reduced due to caching of fhem-svn and docker build cache

  • Upload docker image to github container registry

Todos not possible in this PR

  • Finish upload image to docker hub., Needs to be done in the target repo with the needed rights
  • Update Readme to instruct about available features per supported platform

Relates to: #28

Build docker image via github actions
cache docker build cache to github cache
Added serval vaiabled to supply them to the build job
Addesd caching to fhem svn dir
added fetch depth to get the tags
disable serval image layers as default
Update FROM statement
Use targetplatform to get correct image
Reference build in BUILDARCH arg
limit supported platforms for nodesj 14
Addem some ARG default initialisation
add all plattforms again
support wildcard in branchname
Disabled unsupported of load multiarch image
Comment on lines -6 to -8
ARG BASE_IMAGE
ARG BASE_IMAGE_TAG
ARG ARCH="amd64"
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines make sure these environment variables will still be available after "FROM". They shall not be removed to make sure this information can be accessed later on during runtime (e.g. by dockerinfo module)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are no ENV variables, these are ARG, which are available only during Build process, but not part of the image.
And because i haven't found any location where these ARG are used, i removed them to clean up orphans.

ARG variables are not persisted into the built image as ENV variables are

Copy link
Contributor

Choose a reason for hiding this comment

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

these ARGs are there so people can use them when building a local image themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

you have to repeat BASE_IMAGE and BASE_IMAGE_TAG after FROM, otherwise they will not be available for other commands anymore (e.g. if used by somebody adding some customizations to the build process)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone want's to modify the build process ( dockerfile ) he can add it where he needs it i think.
I removed it, because currently it's nowhere used, but it woud break caching as soon as it is defined with a different value.

Dockerfile Outdated
Comment on lines 403 to 404
RUN if ( [ "${NPM_PKGS}" != "" ] || [ "${IMAGE_LAYER_NODEJS}" != "0" ] || [ "${IMAGE_LAYER_NODEJS_EXT}" != "0" ] ) && ( [ "${ARCH}" == "AMD64" ] || [ "${ARCH}" == "ARMv7" ] || [ "${ARCH}" == "ARM64v8" ] || \
[ ${TARGETPLATFORM} == 'linux/amd64' ] || [ ${TARGETPLATFORM} == 'linux/arm/v7' ] || [ ${TARGETPLATFORM} == 'linux/arm64' ] ) ; then \
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems new variables TARGETARCH and TARGETPLATFORM are used inconsistantly. This shall be corrected before merging into dev branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also make sure the content variable is not tied to be available on Github Actions only as otherwise local builds on personal devices may break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TARGETARCH and TARGETPLATFORM are standard docker variables, noting special to github actions:

https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope

TARGETARCH isn't used on this line, what is your approach?

Dockerfile Outdated
Comment on lines 23 to 37
ARG IMAGE_LAYER_SYS_EXT="1"
ARG IMAGE_LAYER_PERL_EXT="1"
ARG IMAGE_LAYER_DEV="1"
ARG IMAGE_LAYER_PERL_CPAN="1"
ARG IMAGE_LAYER_PERL_CPAN_EXT="1"
ARG IMAGE_LAYER_PYTHON="1"
ARG IMAGE_LAYER_PYTHON_EXT="1"
ARG IMAGE_LAYER_NODEJS="1"
ARG IMAGE_LAYER_NODEJS_EXT="1"

# Custom installation packages
ARG APT_PKGS
ARG CPAN_PKGS
ARG PIP_PKGS
ARG NPM_PKGS
ARG APT_PKGS=""
ARG CPAN_PKGS=""
ARG PIP_PKGS=""
ARG NPM_PKGS=""
Copy link
Contributor

Choose a reason for hiding this comment

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

Be careful changing these to have a default value as on Travis CI, it wouldn't work the way it is now. May be different on Github Actions, to be verified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before i assigned the default values, it was not possible to disable CPAN, PIP, NPM and so on, how did this work before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to get an idea, how the unset AGRG operated before:

buildx call failed with: failed to solve: rpc error: code = Unknown desc = executor failed running [/bin/sh -c if ( [ "${NPM_PKGS}" != "" ] \|\| [ "${IMAGE_LAYER_NODEJS}" != "0" ] \|\| [ "${IMAGE_LAYER_NODEJS_EXT}" != "0" ] ) && ( [ "${ARCH}" == "AMD64" ] \|\| [ "${ARCH}" == "ARMv7" ] \|\| [ "${ARCH}" == "ARM64v8" ] ] \|\| [ ${TARGETPLATFORM} == 'linux/amd64' ] \|\| ${TARGETPLATFORM} == 'linux/arm/v7' ] \|\| ${TARGETPLATFORM} == 'linux/arm64' ] \|\| ); then 

My opinion is, that this comes from the Dockerfile itself how it is preprocessed, but may i am wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set NPM_PKGS to "" becaue it it later tested against "" and not against 0 as the LAYERS.

Comment on lines +133 to +141
IMAGE_LAYER_SYS_EXT=0
IMAGE_LAYER_PERL_EXT=1
IMAGE_LAYER_DEV=0
IMAGE_LAYER_PERL_CPAN=1
IMAGE_LAYER_PERL_CPAN_EXT=0
IMAGE_LAYER_PYTHON=0
IMAGE_LAYER_PYTHON_EXT=0
IMAGE_LAYER_NODEJS=0
IMAGE_LAYER_NODEJS_EXT=0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't exclude any of the layers as it will otherwise break existing configurations.
All layers are essentially needed for maximum compatibility with existing FHEM modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I excluded the layers, because espacally nodejs 14 isn't supported on some of the platforms anymore and this results in different behaviours of the images between platforms.
I am aware of, that this is may be a breaking change for some special cases. But for this cases it is still possible to install the needed packages via docker argument -E.

To save some time during testing an get some minmal working image i decided to disable all those packages to get a pure fhem image which can run fhem similar to the debian package which also has no dependencys to nodejs, python and so on. This redudces also some complexity during setup of the gighub workflow.
Currently the images aren't tested because they are nowhere published. In my oppinion it's best to first check if the base image works on the desired platforms and then move on to extend them again.

For nodejs there are also already available images which may can run the needed services for thos fhem modules who communitcate with a nodejs service . Also the health-check.sh isn't aware of the nodejs services or other laayer, so may it is better to move them to a separate service which is maintained seperate.

IMAGE_LAYER_DEV seems to bring in tools to compile software, which should normaly be not be included in a normal d docker image. Also all this stuff can be added via -E argument if really needed.

Maybe it's worth of providing some -minimal and some -full image variants, depending on the platform. But for now, my goal was to build something of a base image.

Copy link
Contributor

Choose a reason for hiding this comment

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

The different layers for platforms are intentionally to provide maximum possibility on each platform. Providing even more variants is not beneficial in terms of increased maintenance and support effort.

Build tools in the image are required to support FHEM Installer and related components to install Perl modules and other stuff automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the build tools are only needed to install the packages into the image, we can remove them after the installation process is done in a seperate layer.

There is also a job to create a image which has all that services included in the image, same as before. But i think that's not the docker way to have services in different containers.

* build.yml

add build and test step for amd64 plattform

* build.yml

push docker images to github container registry
Update / checkout of fhem improved to hit cache

* Updated build.yml

Build full blown Image

* build.yml

Added unitest after amd64 build
Added "normal" full blown image for supported platform
Added "minimal" image for supported plattforms (without nodejs, python etc)

* build.yml

changed unsuppored conditon

* build.yml

try fix TAG_ROLLING

* Dockerfile

changed Label Title

* build.yml

renamed step

* buidl.yml

Reference recently build image  for tests

* Update build.yml

* build.yml

try to fix unittest

* build.yml

removed unused fhem update from git step
corrected commands and syntax
unittest wait until container is healthy

* Dockerfile

shell syntax error corrected

* Dockerfile

removed architcture and platform check, because this is controlled outside if this can be build

* build.yml

separete test and pushed image into jobs

* build.yml

added missing step to get git vars and cached fhem
disabled chaching because it is useless with this dockerfile

* build.sh

adapted script to usage of buildx insteaf of build
…educed build time (#4)

Dockerfile:

Moved ARG BUILD_DATE and dependend RUNS to the end, becasue this invalidats the cache for every build run

Added Version numbers to every apt package to specify what is installed and be able to update the image if new versions are released. Specifying a new version will invalidate the cache for the layer and all following ones

Added wildcard for some plattform specific versions where version arn't the same overall

Removed upgrade in base envionment, versions should be used from the specified base imae

removed apt-transport-https package, because it's no longer needed

combined COPY to reduce layers created

rearrange dockerfile for improved cache

specified buster als 10.7 to be able to get notified when 10.8 is released via dependabot and start a new build

Use the slim image instead of the full one

build.yml

readded caching, Dockerfile supports now caching
@sidey79
Copy link
Contributor Author

sidey79 commented Jan 2, 2021

@jpawlowski

Did a lot of improvements. I think there is room left, but i think i now need some help to get correct push arguments for docker hub and later on update the readme

May we want to merge this into a feature branch instead of the dev branch to get the work done there

Added extra layer again to copy 99_Dokcerinfo to correct place.
get ip address of default gatway witout ip net tools. Netstat does the job
Added dependabot to this branch
Removed wrong syntax
Changed image Tag to dated version to test if new image from docker hub is detected correctly
@sidey79
Copy link
Contributor Author

sidey79 commented Feb 21, 2021

@jpawlowski

Wie kommen wir hier weiter?
Willst Du die Repo Rechte ggf. weitegeben?

Copy link
Contributor

@jpawlowski jpawlowski left a comment

Choose a reason for hiding this comment

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

@jpawlowski

Wie kommen wir hier weiter?
Willst Du die Repo Rechte ggf. weitegeben?

Ehrlich gesagt gefallen mir die Änderungen so überhaupt nicht. Die Abhängigkeiten zum FHEM Installer und den anderen FHEM Modulen wurden gar nicht berücksichtigt. Auch werden Änderungen vermischt, die mit dem Wechsel auf Github Actions nichts zu tun haben.

@@ -14,7 +14,7 @@ RESTART="${RESTART:-1}"
TELNETPORT="${TELNETPORT:-7072}"
CONFIGTYPE="${CONFIGTYPE:-"fhem.cfg"}"
DNS=$( cat /etc/resolv.conf | grep -m1 nameserver | sed -e 's/^nameserver[ \t]*//' )
export DOCKER_GW="${DOCKER_GW:-$(ip -4 route list match 0/0 | cut -d' ' -f3)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you mix this change into the Travis>GHActions migration path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why is this method more reliable and what made you changing this command? pls explain so we have it documented as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this, to avoid a additional package dependency in the image. The ip command comes via iproute2
It's mixed this in here, because this package isn't in the install list, so i changed to available commands.

@@ -93,6 +102,7 @@ docker build \
--build-arg IMAGE_VCS_REF=${TRAVIS_COMMIT} \
Copy link
Contributor

Choose a reason for hiding this comment

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

pls find a proper replacement for this variable as it is required to keep build information as part of the image (git commit id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like git rev-parse --short HEAD ?
I haven't included this, because in my oppinion the behaivior is the same as bevore, because TRAVIS_COMMIT isn't set somewhere inside this script, so if somebody called that script before, this was an empty variable.

Comment on lines -6 to -8
ARG BASE_IMAGE
ARG BASE_IMAGE_TAG
ARG ARCH="amd64"
Copy link
Contributor

Choose a reason for hiding this comment

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

these ARGs are there so people can use them when building a local image themselves.

Comment on lines -6 to -8
ARG BASE_IMAGE
ARG BASE_IMAGE_TAG
ARG ARCH="amd64"
Copy link
Contributor

Choose a reason for hiding this comment

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

you have to repeat BASE_IMAGE and BASE_IMAGE_TAG after FROM, otherwise they will not be available for other commands anymore (e.g. if used by somebody adding some customizations to the build process)

Comment on lines +133 to +141
IMAGE_LAYER_SYS_EXT=0
IMAGE_LAYER_PERL_EXT=1
IMAGE_LAYER_DEV=0
IMAGE_LAYER_PERL_CPAN=1
IMAGE_LAYER_PERL_CPAN_EXT=0
IMAGE_LAYER_PYTHON=0
IMAGE_LAYER_PYTHON_EXT=0
IMAGE_LAYER_NODEJS=0
IMAGE_LAYER_NODEJS_EXT=0
Copy link
Contributor

Choose a reason for hiding this comment

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

The different layers for platforms are intentionally to provide maximum possibility on each platform. Providing even more variants is not beneficial in terms of increased maintenance and support effort.

Build tools in the image are required to support FHEM Installer and related components to install Perl modules and other stuff automatically.

Comment on lines -389 to -392
python3-pychromecast \
speedtest-cli \
youtube-dl \
&& ln -s ../../bin/speedtest-cli /usr/local/bin/speedtest-cli \
Copy link
Contributor

Choose a reason for hiding this comment

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

pinning was intentionally left out because there is no real value not to use the latest and greatest version...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pinning is implemented, to invalidate the cache if a new version should be uses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here a reference what can bei the result If not aware of a compatible Version:

https://forum.fhem.de/index.php/topic,119201.0.html

Comment on lines -377 to -382
libinline-python-perl \
python3 \
python3-dev \
python3-pip \
python3-setuptools \
python3-wheel \
Copy link
Contributor

Choose a reason for hiding this comment

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

pinning was intentionally left out because there is no real value not to use the latest and greatest version...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pinning is implemented, to invalidate the cache if a new version should be uses


# Add Perl extended app layer for pre-compiled packages
RUN if [ "${IMAGE_LAYER_PERL_EXT}" != "0" ]; then \
LC_ALL=C DEBIAN_FRONTEND=noninteractive apt-get update \
&& LC_ALL=C DEBIAN_FRONTEND=noninteractive apt-get install -qqy --no-install-recommends \
perl \
Copy link
Contributor

Choose a reason for hiding this comment

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

again, no pinning here. the intention is to rely on the latest version of the upstream environment.
FHEM Installer and Updater Modules would otherwise also complain about existing updates. Does not make sense to always be behind with updates even when pulling the latest image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pinning is needed, to invalidate the cache if there is a new version. Otherwise if the layer doesn't change, the cache is never invalidated.

I'm aware, that there needs to be some update jobs done. Currently i'm hoping that this will get into dependabot soon, so every time a new package is releases, a new build can be triggered to be up to date.
With the old behaivour, i think, there was no information available what versions are installed, and if somebody builds it on his own, he will get different results for the same dockerfile and revision, if the build job is run on a different time.

dependabot bot and others added 6 commits March 28, 2021 21:41
Bumps [actions/cache](https://github.com/actions/cache) from v2 to v2.1.4.
- [Release notes](https://github.com/actions/cache/releases)
- [Commits](actions/cache@v2...26968a0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/cache](https://github.com/actions/cache) from v2.1.4 to v2.1.5.
- [Release notes](https://github.com/actions/cache/releases)
- [Commits](actions/cache@v2.1.4...1a9e213)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/setup-qemu-action](https://github.com/docker/setup-qemu-action) from v1.0.1 to v1.1.0.
- [Release notes](https://github.com/docker/setup-qemu-action/releases)
- [Commits](docker/setup-qemu-action@v1.0.1...c308fdd)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updated outdated packages

ca-certificates
dnsutils
subversion
sudo
unzip
 - Fixed dependecys
 - Updarted to recent baseimage
@CoolTuxNet CoolTuxNet changed the base branch from dev to patch_marko-bullseye October 17, 2021 16:34
@CoolTuxNet CoolTuxNet merged commit 9f672cb into fhem:patch_marko-bullseye Oct 17, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants