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

Added support for Docker #3

Merged
merged 5 commits into from
Jul 17, 2020
Merged

Conversation

davidbonnici1984
Copy link

Please review all changes including changes to Chromium installation

@b1f6c1c4 b1f6c1c4 self-requested a review June 20, 2020 09:33
@b1f6c1c4 b1f6c1c4 self-assigned this Jun 20, 2020
@b1f6c1c4 b1f6c1c4 added the enhancement New feature or request label Jun 20, 2020
Dockerfile Outdated
COPY . .
RUN npm install
VOLUME ["/files"]
ENTRYPOINT [ "sh", "convert.sh" ]
Copy link
Owner

Choose a reason for hiding this comment

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

Missing final EOL.

convert.sh Outdated
Comment on lines 7 to 9
if test -f "${dir}.pdf"; then
rm $dir.pdf
fi
Copy link
Owner

Choose a reason for hiding this comment

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

It's a bad practice to test and then rm. Instead, always use rm -f.

convert.sh Outdated
Comment on lines 12 to 14
if test -f "${dir}.png"; then
rm $dir.png
fi
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, use rm -f

.gitignore Outdated

./project
project
package-lock.json
Copy link
Owner

Choose a reason for hiding this comment

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

package-lock.json shall NOT be put into .gitignore. This repo is using yarn, but the choice was made back in 2018. I moved to npm on master and plz rebase.

.dockerignore Outdated
Dockerfile
.git
./files/*
.gitignore
Copy link
Owner

Choose a reason for hiding this comment

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

Missing final EOL.

Dockerfile Outdated
RUN mkdir -p /home/node/draw.io-export /files
WORKDIR /home/node/draw.io-export
COPY . .
RUN npm install
Copy link
Owner

Choose a reason for hiding this comment

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

Use npm ci --only=production instead of npm install.

Copy link
Owner

Choose a reason for hiding this comment

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

Further, consider COPY package*.json ./ first before COPY . ., as stated in this guide.

Dockerfile Outdated
Comment on lines 1 to 5
FROM node:lts-alpine3.11
RUN apk add chromium
ENV PUPPETEER_SKIP_CHROMIUM_DOWNLOAD true
ENV CHROMIUM_PATH /usr/bin/chromium-browser
RUN mkdir -p /home/node/draw.io-export /files
Copy link
Owner

Choose a reason for hiding this comment

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

This part looks OK to me.

convert.sh Outdated
@@ -0,0 +1,17 @@
echo "DRAW.IO-EXPORT Started."
Copy link
Owner

Choose a reason for hiding this comment

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

Missing #!/bin/bash

convert.sh Outdated
Comment on lines 2 to 5
dirs=$(find /files -name '*.drawio')

for dir in $dirs
do
Copy link
Owner

Choose a reason for hiding this comment

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

This scripts will fail if any of the files contains "abnormal" characters, like spaces.
Please consider write a separate script file convert-one.sh and do :

find /files -name '*.drawio' -exec ./convert-one.sh {} \;

Alternatively, you can try combining the two scripts together and, at the beginning of the script, check [ "$#" -eq 1 ] to know if the script is called by find or by docker.

export.js Show resolved Hide resolved
@b1f6c1c4 b1f6c1c4 merged commit fa32c06 into b1f6c1c4:master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants