Conversation
…lightly improved error reporting
|
What's the status on this? :) |
|
Hi, sorry I didn't took time to review and merge this PR I'll work on it ASAP |
|
@fleebzz I check the code and tested the changes, there are few things that can be improved, I'll add comments to code |
|
Thx @VeeeneX ! I started to run this version and looks like to work fine. I'll take look to your comments. |
VeeeneX
left a comment
VeeeneX
left a comment
There was a problem hiding this comment.
I have small suggestions as well as add start script to package.json
"scripts": {
"start": "node index.js"
},| && apt install -y libxinerama1 libfontconfig1 libdbus-glib-1-2 libcairo2 libcups2 libglu1-mesa libsm6 unzip \ | ||
| && tar -zxvf libo.tar.gz | ||
| WORKDIR LibreOffice_6.3.3.1_Linux_x86-64_deb/DEBS | ||
| WORKDIR LibreOffice_6.4.5.2_Linux_x86-64_deb/DEBS |
There was a problem hiding this comment.
Workdirs should not be used like this, I prefer this example:
FROM node:lts
WORKDIR /carbone-api
RUN apt update \
&& apt install -y libxinerama1 libfontconfig1 libdbus-glib-1-2 libcairo2 libcups2 libglu1-mesa libsm6 unzip
RUN wget http://downloadarchive.documentfoundation.org/libreoffice/old/6.4.5.2/deb/x86_64/LibreOffice_6.4.5.2_Linux_x86-64_deb.tar.gz -O libo.tar.gz \
&& tar -zxvf libo.tar.gz
RUN cd LibreOffice_6.4.5.2_Linux_x86-64_deb/DEBS && dpkg -i *.deb
COPY . /carbone-api
RUN yarn install --production
CMD node index
There was a problem hiding this comment.
I could agree just : what's the point but adding a slow COPY step ?
There was a problem hiding this comment.
index.js has to be copied into container right?
There was a problem hiding this comment.
Sorry misread your proposal. Now I can say I disagree 😛
From dockerfile_best-practices :
For clarity and reliability, you should always use absolute paths for your WORKDIR.
Also, you should use WORKDIR instead of proliferating instructions like RUN cd … && do-something, which are hard to read, troubleshoot, and maintain.
There was a problem hiding this comment.
Ok :DDD Agree anyway we can use that node:lts and rest 😄
Thank you!
| @@ -1,6 +1,5 @@ | |||
| const path = require(`path`); | |||
| const fs = require(`fs-extra`); | |||
There was a problem hiding this comment.
We can replace fs extra and remove extra dependency in flavor of:
const fs = require(`fs`).promises;| return res.status(500).send(`Internal server error: ${e}`); | ||
| } | ||
|
|
||
| fs.remove(template.path); |
There was a problem hiding this comment.
This is related to native fs dependency
await fs.unlink(template.path);
| "body-parser": "^1.19.0", | ||
| "carbone": "^2.0.0", | ||
| "express": "^4.17.1", | ||
| "fs-extra": "^9.0.1", |
There was a problem hiding this comment.
Sorry didn't see the other comment about the native promises
There was a problem hiding this comment.
Yes and can be replaced with native build in const fs = require('fs').promises; so we remove extra dependency and remove few kb 😄
|
And last note we can use /tmp instead of |
|
wow. what a sudden burst of activity. am i right that all of suggestions are related to original code, not to my changes? |
|
@bsl-zcs Mainly, only few anyway thanks for changes! |
3 participants
fixes #1