-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add Asciidoctor Web PDF #244
base: main
Are you sure you want to change the base?
Conversation
Hi @diguage thanks for the contribution. Is there an issue that could help us (maintainers) to understand a bit better the "why/what/how" of this PR please? A few feedback at first:
|
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.
Some important feedback to allow this PR to land (version tracking is mandatory)
@@ -100,7 +102,8 @@ RUN apk add --no-cache \ | |||
tzdata \ | |||
unzip \ | |||
which \ | |||
font-noto-cjk | |||
font-noto-cjk \ | |||
nodejs |
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 is the version of nodejs used here? Could you fix it and add an updatecli manifest (in
./updatecli/updatecli.d/
) to track its version please? - Could you share, in the PR description, the additional size resulting with this change (the image is already quite heavy)?
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.
OK, let me try it. I do not know how to do it(the updatecli manifest), give me some time.
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.
No problem: it is not very well documented on the contributing section so there is no strong expectations.
My message was to let you know that it is needed but I may help.
I suggest that you try by checking the existing manifests.
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 updated the code. But it is not the final work. Give me some time.
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 updated the code to add a test case and manifest for Node.js. Please check it. If it has some errors, please help me to update it.
I tried to build it many times but it always threw errors. Please help me to check the additional size resulting with this change. Thanks.
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.
WDYT about using a Multi stage build with the official NodeJS image (instead of instaling the nodejs package) in which you install the NPM packages, and you retrieve only the required directories/files (explained in https://medium.com/geekculture/how-to-install-a-specific-node-js-version-in-an-alpine-docker-image-3edc1c2c64be for a "pure" NodeJS installation).
It would allow faster builds (node js / npm run in parallel of the rest), and a fixed controlled version of nodejs.
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 me try it.
This issue is related to #183. Please see my comment there. |
@@ -28,6 +28,8 @@ This Docker image provides: | |||
|
|||
- [Asciidoctor Kroki](https://github.com/Mogztter/asciidoctor-kroki) 0.5.0 | |||
|
|||
- [Asciidoctor Web PDF](https://github.com/Mogztter/asciidoctor-web-pdf) 1.0.0-alpha.14 -- This is an unofficial project. |
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.
@mojavelinux I add the description: This is an unofficial project.
. Is it OK?
@@ -34,6 +43,7 @@ This Docker image provides: | |||
* https://github.com/asciidoctor/asciidoctor-confluence[Asciidoctor Confluence] {ASCIIDOCTOR_CONFLUENCE_VERSION} | |||
* https://github.com/asciidoctor/asciidoctor-bibtex[Asciidoctor Bibtex] {ASCIIDOCTOR_BIBTEX_VERSION} | |||
* https://github.com/Mogztter/asciidoctor-kroki[Asciidoctor Kroki] {ASCIIDOCTOR_KROKI_VERSION} | |||
* https://github.com/Mogztter/asciidoctor-web-pdf[Asciidoctor Web PDF] {ASCIIDOCTOR_WEB_PDF_VERSION} -- This is an unofficial project. |
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.
@mojavelinux I add the description: This is an unofficial project.
. Is it OK?
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.
Please help me to check or update the test case and the updatecli manifest. Thanks a lot.
@@ -12,6 +12,9 @@ ARG asciidoctor_revealjs_version=4.1.0 | |||
ARG kramdown_asciidoc_version=2.0.0 | |||
ARG asciidoctor_bibtex_version=0.8.0 | |||
ARG asciidoctor_kroki_version=0.5.0 | |||
ARG nodejs_version=16.14.0-r0 | |||
ARG asciidoctor_web_pdf_version=1.0.0-alpha.14 | |||
ARG asciidoctor_kroki_npm_version=0.15.4 |
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 is the reason to add this element (since kroki is already installed)?
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.
Asciidoctor Kroki has two versions: for Ruby and for Node.js. Asciidoctor Kroki for Ruby was installed. But Asciidoctor Kroki for Node.js is not installed.
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.
Gotcha, thanks for the explanation! In that case we'll have to track the version of this component as well with an updatecli manifest :) (yea it's cumbersome but the goal is ensure that control what is inside this image to avoid supply chain issues)
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
The PR contains two changes: