Carbone 2.0 #2
Carbone 2.0 #2
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. |
| RUN apt update \ | ||
| && 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;| @@ -56,7 +60,7 @@ app.post('/render', upload.single(`template`), async (req, res) => { | |||
| report = await render(template.path, data, options); | |||
| } catch (e) { | |||
| console.log(e); | |||
| return res.status(500).send(`Internal server error`); | |||
| 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! |
fixes #1