-
Notifications
You must be signed in to change notification settings - Fork 123
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
Elektra Web - docker images #2100
Elektra Web - docker images #2100
Conversation
USER elektra | ||
|
||
# create start script | ||
RUN printf "#!/bin/bash\nkdb run-elektrad &\nkdb run-web" > start |
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 thought you will split this up into two containers?
Please give a description about the individual dockerfiles. (as comments and in the README.md)
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.
@markus2330 I explained this in the PR description, elektra/web
contains both so that you can quickly test elektra web (quickstart guide in README)
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 added the description to a README file
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.
Thank you! Please always keep all information within the repo (and not in the PR description).
And please do not forget to update release notes. |
@markus2330 the docker images are already mentioned in the #2099 PR release notes |
They need to be in every PR individually so that the build server passes. |
@markus2330 alright, I added them here too |
@ingwinlu Can you review this please? Would be great if the demo is updated to the latest version. |
scripts/docker/web/base/Dockerfile
Outdated
# pull latest libelektra | ||
RUN mkdir /home/elektra | ||
WORKDIR /home/elektra | ||
RUN git clone https://github.com/omnidan/libelektra.git |
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.
Don't you want to use ElektraInitiative's repo here?
you also want git clone --branch my_abc http://git.abc.net/git/abc.git and probably a shallow clone.
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.
you are right, I will do that now that the other PR is merged and rebuild the images
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 like to get .tar.gz archives from github. This removes the need to install git and it makes it very easy to insert a build-arg that represents the version I want to build.
# mount copy of /etc/hosts to user/hosts | ||
|
||
# mount as root user | ||
RUN kdb mount --with-recommends hosts user/hosts hosts |
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.
will that work? shouldn't you be the elektra user when inheriting from your base image?
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.
yes, USER
is not called in the base image
RUN kdb setmeta user/app/peers/#0 check/validation/message "invalid ip address" | ||
RUN kdb setmeta user/app/peers/#1 check/validation "^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$" | ||
RUN kdb setmeta user/app/peers/#1 check/validation/message "invalid ip address" | ||
RUN kdb set user/app/peers/#1 "192.168.0.44" |
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.
what are the ip addresses used here?
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.
they are just an example for the demo, they have no meaning
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.
in that case: you could provide a file containing the demo data and mount that inside the container. would reduce image layers by a lot and is probably easier to maintain in the long run (maybe you can even use something that is around in the repository for tests)
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 added some comments.
Overall you could reduce image size a lot by reducing layers (chaining commands together).
You could also move everything in one Dockerfile via multi stage builds.
I have no idea what the hardcoded ip addresses are for.
I updated, rebuilt & pushed all of the images. Thanks for the review and recommendations @ingwinlu |
|
scripts/docker/web/base/Dockerfile
Outdated
# pull latest libelektra | ||
RUN mkdir /home/elektra | ||
WORKDIR /home/elektra | ||
RUN git clone --depth 1 https://github.com/ElektraInitiative/libelektra.git |
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.
You should pin a version for reproducible builds.
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.
you did this before by checking out the tag. I would go for a build-arg that you feed to a --branch parameter of clone (you can let it default to master if you want).
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.
@ingwinlu good idea, I did that now!
@markus2330 can you try I think the problem is that I pushed a new image over the same tag, so maybe your docker thinks it already has the latest version. Similar to this issue: moby/moby#13331 |
@markus2330 add the --pull flag |
@ingwinlu I pinned the version and added a build argument for it |
elektra/web:1.5 downloads something but how can I know the difference? It always shows "v1.5" and rename works. |
Thank you for the review and the new dockerfiles! |
@markus2330 if it shows |
I created (and pushed) the following docker images (as per #1901):
elektra/web-base
- base image for elektra web, all other images build upon this (builds elektra withyajl
andkdb
)elektra/elektrad
- image that only starts elektradelektra/webd
- image that only starts webdelektra/elektrad-demo
- same aselektrad
, but with a KDB config set up (for demo, should run on http://elektrad-demo.libelektra.org)elektra/webd-demo
- same aswebd
, but with 2 instances already created, they both connect to http://elektrad-demo.libelektra.org with different visibility levels (for demo, should run on http://webui.libelektra.org)elektra/web
- image that starts elektrad & webd (for those who just want to try out Elektra Web locally, also mentioned in quickstart in README of Elektra Web 1.5 (fixed) #2099)All images are available under the
1.5
andlatest
tags.You can find the images on Docker Hub: https://hub.docker.com/u/elektra/
@markus2330 @ingwinlu please let me know your Docker Hub usernames so that I can add you as owners to the
elektra
group