-
Notifications
You must be signed in to change notification settings - Fork 12
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
Docker Hub Image #37
base: master
Are you sure you want to change the base?
Docker Hub Image #37
Conversation
cc0a42c
to
61c8f37
Compare
Anyone reading this has enough Docker experience/knowledge to be able to review this PR? Any and all help would be most appreciated! 🙏 |
I prefer standalone docker files, i.e. reduces build steps and errors on the end user side but this is not needed for this project IMHO I would suggest using this My DockerfileFROM alpine:latest AS appBuild
LABEL maintainer="Peponi <[email protected]>" \
description="will run the Armadietto NodeJS web service (a RemoteStorageJS backend)"
ARG PROJECT_NAME="armadietto"
ARG PORT="8000"
ARG PKG_MANAGER="yarn"
ARG INSTALL_COMMAND="yarn global add"
ARG CONFIG_PATH_CERTS="/etc/letsencrypt/live/example.com/"
ARG CONFIG_PATH_STORAGE="/usr/share/armadietto"
ENV PROJECT_NAME=$PROJECT_NAME
RUN set -e;\
apk add \
curl \
git \
nodejs \
$PKG_MANAGER; \
mkdir /opt/armadietto; \
mkdir -m 0700 /$CONFIG_PATH_STORAGE; \
mkdir -m 0700 /$CONFIG_PATH_CERTS; \
$INSTALL_COMMAND $PROJECT_NAME; \
apk del git $PKG_MANAGER; \
adduser -u 6582 -HD $PROJECT_NAME;
COPY config.json /etc/armadietto.conf.json
VOLUME $CONFIG_PATH_STORAGE $CONFIG_PATH_CERTS
USER $PROJECT_NAME
WORKDIR ~
EXPOSE $PORT
CMD $PROJECT_NAME -c /etc/armadietto.conf.json
HEALTHCHECK CMD curl --fail http://127.0.0.1:$PORT/ || exit 1
### Install ###
#
# BUILD:
#
# default for amd64 architecture
#
# > docker build -t armadietto:latest .
# > docker build -t --build-arg PKG_MANAGER="npm" --build-arg INSTALL_COMMAND="npm i -g" armadietto:latest .
#
# RUN:
#
# > docker run --rm -p 8000:8000 rarmadietto:latest
#
# INFO: config.json needs to be byside the Dockerfile
#
# {
# "allow_signup": true,
# "storage_path": "/usr/share/armadietto",
# "cache_views": true,
# "http": {
# "host": "0.0.0.0",
# "port": 8000
# },
# "https": {
# "enable": false,
# "force": false,
# "port": 4443,
# "cert": "/etc/letsencrypt/live/example.com/cert.pem",
# "key": "/etc/letsencrypt/live/example.com/privkey.pem"
# },
# "basePath": ""
# }
#
### e2e testimage sizeEPOSITORY TAG IMAGE ID CREATED SIZE
armadietto peponi 9a84b73851eb 21 minutes ago 52.4MB
armadietto bytesnz 9829e4ad797b About an hour ago 118MB
security scan with trivy ✘ ~/Code/private/DockerFiles/armadietto docker run --rm -v /var/run/docker.sock:/var/run/docker.sock -v cache:/root/.cache/ aquasec/trivy armadietto:peponi
2021-05-23T09:21:40.833Z INFO Detected OS: alpine
2021-05-23T09:21:40.833Z INFO Detecting Alpine vulnerabilities...
2021-05-23T09:21:40.836Z INFO Number of PL dependency files: 1
2021-05-23T09:21:40.836Z INFO Detecting yarn vulnerabilities...
armadietto:peponi (alpine 3.13.5)
=================================
Total: 0 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 0)
usr/local/share/.config/yarn/global/yarn.lock
=============================================
Total: 0 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 0)
|
I just created a Maybe someone more experienced wants access to the RS team there? The free/community plan allows 2 more accounts to be added... Edit: considering that we're stuck on GitHub for now, would it also make sense to consider GitHub's own Docker registry for publishing this? Seems to me like Docker Hub is rather crippled on free plans. |
Cool. Looks good. Will integrate some of the stuff with the MR. The reason I went for using copy instead of yarn install is for integrating with github actions I - it would need to be published to npm before the docker image is created, but that's doable. I think it is good to have a docker image as well as it makes it super easy to deploy. |
We may want to utilize GitHub actions to do the final build/push to docker hub as well. I'd be in favour of remaining on docker hub, but if we want we can always push the image to multiple locations. |
👍 The MR has a template Github Action for building docker images, but it will need some tweaking. |
@bytesnz Did you find some time to integrate the things you wanted by any chance? No pressure, just making sure the PR doesn't go stale... |
@raucao I did start making changes and looking at it. I unfortunately ran out of time before going on a trip. I was going to merge the two dockerfiles and add testing to the action from peponi. I can do this at the start of Sep (long trip without a laptop). |
- Fix healthcheck - Switch back to using local files for creation instead of global install - Use build container for installing node modules - Remove docs from Dockerfile
0bbb030
to
2b7fffd
Compare
Sorry about the delay. Have done a bit of merging between the two Dockerfiles:
For the github actions job, would be good to add an e2e test run to it as well, but would require a bit of test |
FROM alpine:latest | ||
|
||
ARG CONFIG_PATH_STORAGE="/usr/share/armadietto" | ||
ARG PROJECT_NAME="armadietto" |
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.
This line is doubled (see line 5).
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.
One for each container (build and the main container), thought it isn't used in the build container, so can delete it... done.
docker/Dockerfile
Outdated
@@ -19,6 +16,8 @@ RUN $INSTALL_COMMAND | |||
|
|||
FROM alpine:latest | |||
|
|||
LABEL description="Armadietto NodeJS web service (a RemoteStorageJS backend)" |
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.
remoteStorage.js is only one client library for using/integrating the remoteStorage protocol. I'd suggest something like:
LABEL description="Armadietto NodeJS web service (a RemoteStorageJS backend)" | |
LABEL description="Armadietto node.js web service (a remoteStorage server)" |
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.
Looks good to me 👍
6354401
to
0ed6413
Compare
# armadietto [![Build Status](https://secure.travis-ci.org/remotestorage/armadietto.svg)](http://travis-ci.org/remotestorage/armadietto) [![js-semistandard-style](https://img.shields.io/badge/code%20style-semistandard-brightgreen.svg?style=flat-square)](https://github.com/Flet/semistandard) [![Codacy Badge](https://api.codacy.com/project/badge/Grade/0eaafdf96ebb47a9ac462bcf6a7ccb06)](https://www.codacy.com/app/lesion/armadietto?utm_source=github.com&utm_medium=referral&utm_content=remotestorage/armadietto/&utm_campaign=Badge_Grade) | ||
|
||
> ### :warning: WARNING | ||
> Please do not consider `armadietto` production ready, this project is still |
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.
Not an issue with this PR, but wondering out loud, what is the gate at which we consider Armadietto production ready? I think hard to assess how many production workloads are already running on top of it?
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.
Good question! I think it merits its own issue or forums topic.
@bytesnz, was testing this a bit. Pulled your branch and...
But it exits right away with So I went into the image:
And running the linked Are we certain we can start the process like that? I've only started it as a library as per Would love to get this out and build on top of it. I see there are about 15k downloads of armadietto on docker-hub from various providers, would be nice to get this official one out 👍. |
Hey @JakubNer. Hmm, that's weird. Just tried running through the build process again and no issues for me (commands below). Is the image bytesnz/armadietto:latest working for you (is built off this repo? git clone https://github.com/bytesnz/armadietto
cd armadietto/
git checkout add-dockerfile
docker build -t remotestorage/armadietto:latest -f ./docker/Dockerfile .
docker run -p 8000:8000 --name armadietto remotestorage/armadietto:latest |
@bytesnz, indeed when I pull your image ( My own build fails like I mentioned. I did build off of the right place:
Just rebuilt it locally, same error. Weird. |
That's a very nice blog post! But I'm not sure we should keep extending the Docker pull request with more unrelated information or tasks. I think the only thing missing here are final reviews. |
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.
Let's get this in.
After rebasing on master, it would be nice to verify that logs can be collected outside the container, but that doesn't have to happen before this is merged. |
@DougReeder Want to give this a final review and approve and merge it in case everything works and looks good? |
- Use npm instead of yarn - Add a log directory - Update to the latest example config
Sorry for the last minute. Was just looking at testing the merge from master and saw it may have no conflicts, but there are some breaking changes (my bad @raucao et al for not looking properly at it earlier). Nice update to the server pages btw. Have done a commit to fix the changes - adding a log directory, using npm instead of yarn, update the docker config to the latest example config. Also saw the logging has been added and added to the example config. The Dockerfile(build) is currently using a static config file docker/config.json. If the http port isn't going to change, I could update the Dockerfile to run |
The "Docker Image CI" workflow is failing for me in the buildx failed with: error: invalid tag "/armadietto:add-dockerfile": invalid reference format Does some configuration need to be set in the remotestorage/armadietto repository to make this work? |
@DougReeder, yeah, the |
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.
@bytesnz, I verified your latest merge including the new logging and new login page.
Synced your branch and...
docker build -t remotestorage/armadietto:latest -f .\docker\Dockerfile .
docker run -d -p 8000:8000 remotestorage/armadietto:latest
docker log ... --follow
Logs do show up and the new page does pop up.
BTW, I still had to change the CMD
line in Dockerfile
from CMD /opt/armadietto/bin/armadietto.js -c /etc/armadietto.conf.json
to CMD node /opt/armadietto/bin/armadietto.js -c /etc/armadietto.conf.json
🤷♂️. Regardless, I know the images work fine when github updates (from previous testing).
docker/Dockerfile
Outdated
|
||
RUN ln -s /opt/armadietto/bin/armadietto.js /usr/local/bin/armadietto | ||
|
||
COPY docker/config.json /etc/armadietto.conf.json |
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.
Setting this up in k8s and I find myself changing /etc
to /usr/local/etc
to make it easier to mount my own config without clobbering the rest of the /etc
folder: data file mount.
Maybe we can keep this as /usr/local/etc/..
?
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.
Yeah, I agree with the not clobbering /etc. I did it based on the README, which has changed to /etc/armadietto/conf now (so should update it in this...?). With the docker image, you can store the conf file wherever you want, it just needs to be mounted in /etc, so it isn't too bad the way it is atm (in /etc)?
Sorry for the delayed response. I finally got around to trying out building the docker image in windows and found the issue 😞 it's the line endings. When you check it out on windows (depending on your settings), it will change the line ending to CRLF, which, turns out, the bang line does not like. Changing the line endings back to just LF fixes the problem. 😞
|
I've updated the Dockerfile to ensure the bin file (bin/armadietto.js) always has unix line endings on the docker image, so you shouldn't have anymore issues on windows @JakubNer. I've also updated the location of the conf file so it is the same as the current README. Hopefully we can get this over the line. |
…nto add-dockerfile
AFAICS, this PR just needs a final review from @JakubNer for Windows compatibility, correct? |
So far as I can tell, this PR could still be merged as-is. It would be nice to merge in https://github.com/bytesnz/armadietto/pull/7/files first, though. |
@bytesnz Could you have a look at @DougReeder's PR to your repo? |
Ensures Docker image uses tested package versions
reviewed and merged 👍 |
Now we just need to merge master into this, to ensure we won't lose any of the changes to the submitBtn and documentation. |
Just starting to add remote-storage (RS) to some projects I am working on, so am setting up my own RS server. I didn't see that you don't have a (recent) docker image on docker hub, so created a Dockerfile and a Github Action to build and publish one when a release is created.
The image is very basic - uses the node:lts-alpine docker image and the basic armadietto config (from
armadietto -e
). It exposes the armadietto http port to be used with a proxy.Are you interested in including this in the repo? Figure you could create an organisation on docker hub to publish it under.
Need to at least:
X-Forwarded-Host
is set)yarn global
/npm i -g