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

Add stage with minimal image #187

Merged
merged 1 commit into from
May 1, 2021
Merged

Add stage with minimal image #187

merged 1 commit into from
May 1, 2021

Conversation

reitzig
Copy link
Contributor

@reitzig reitzig commented Mar 1, 2021

For simple use cases, a smaller image than the full ~450MB can suffice.

This MR adds a separate build stage with a minimal image of ~40MB that contains the bare minimum to run asciidoctor(-pdf); everything else would have to be added usage-site.

$ make build
<snip>
$ docker run --rm asciidoctor/docker-asciidoctor:multi-stage-build-minimal sh -c "asciidoctor --version && asciidoctor-pdf --version"
Asciidoctor 2.0.12 [https://asciidoctor.org]
Runtime Environment (ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux-musl]) (lc:UTF-8 fs:UTF-8 in:UTF-8 ex:UTF-8)
Asciidoctor PDF 1.5.4 using Asciidoctor 2.0.12 [https://asciidoctor.org]
Runtime Environment (ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux-musl]) (lc:UTF-8 fs:UTF-8 in:UTF-8 ex:UTF-8)

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Hello @reitzig , thanks a lot for this proposal!

Given this work, what do you think we update it to solve #79 ?

The proposal is the following:

  • The minimal image should be name asciidoctor/asciidoctor and its tag should be the asciidoctor's version
  • The "kitchen sink" image, which is currently the asciidoctor/docker-asciidoctor image, should be now named asciidoctor/box

If it is ok for you, I'll let you update the PR, and I'll create the associated Docker Repositories in the DockerHub?

@slonopotamus
Copy link
Contributor

What's the reasoning behind inclusion of asciidoctor-pdf in minimal image?

@slonopotamus
Copy link
Contributor

slonopotamus commented Mar 2, 2021

the full ~450MB

I currently see that latest image is 233MB. Not sure though what has happened after 1.3.2 (450MB) that affected image size so much.

UPD: #184 happened.

@dduportal
Copy link
Contributor

dduportal commented Mar 2, 2021

@slonopotamus be careful as the size reported on the DockerHub are compressed sizes, while I understand that @reitzig is speaking in term of the size reported by the Docker Engine (uncompressed I assume):

➜ docker image ls | grep ascii
asciidoctor/docker-asciidoctor   1.3.3                 a03ff86dd566   12 hours ago   452MB
➜ docker save asciidoctor/docker-asciidoctor:1.3.3 | gzip > adoc.gz
➜ du -sh adoc.gz 
240M    adoc.gz

@reitzig
Copy link
Contributor Author

reitzig commented Mar 3, 2021

@dduportal : If it is ok for you, I'll let you update the PR, and I'll create the associated Docker Repositories in the DockerHub?

Can do. Do the maintainers agree about those tags?

@slonopotamus : What's the reasoning behind inclusion of asciidoctor-pdf in minimal image?

Figured being able to generate PDFs would be prudent for a base image.

@dduportal
Copy link
Contributor

@reitzig

Can do. Do the maintainers agree about those tags?
Sure, it was already discussed in #79 and #72 we can go for it :)

What's the reasoning behind inclusion of asciidoctor-pdf in minimal image?
Figured being able to generate PDFs would be prudent for a base image.

I'm not really sure about adding or removing asciidoctor-pdf in the "minimalistic" image. Do you have an advise on this @mojavelinux ?

@mojavelinux
Copy link
Member

I'm not really sure about adding or removing asciidoctor-pdf in the "minimalistic" image. Do you have an advise on this @mojavelinux ?

That's a tough call. There are reasonable arguments for or against including it.

The argument against including it is that the base / minimal image should have only the Asciidoctor gem and nothing else so it's the absolute least common denominator. And while there's certainly a case to be made for that, it ends up being so minimal that you have to wonder why we would even need an image then because it's hardly different than the public Ruby image(s).

The argument for including it is that the base / minimal image should provide tools for the most common publishing workflows. And for that, PDF is an absolute must. Like it or not, nearly everyone ends up needing PDF output for something. And since it only depends on a handful of other gems, it adds very little size to the image. You could definitely make the case to include it, but not include it's optional system dependencies, ghostscript and graphicsmagick (which aren't included anyway).

I think the right decision is to include Asciidoctor PDF (at least until we have a suitable replacement).

@dduportal
Copy link
Contributor

Thanks @mojavelinux for this clear explanation!

I just had a thought about the versioning of the minimalistic image that could be an argument against adding Asciidoctor PDF: if we want to keep the image tag to the asciidoctor image, then it means that both the upstream distribution (OS version, ruby version) AND the asciidoctor-pdf version are seen as "always the latest compatible".

for instance: given the image asciidoctor/asciidoctor:2.0.12 , I expect to always have the latest Alpine Linux and latest Ruby version compatible with Asciidoctor v2.0.12.
But what about Asciidoctor PDF? Should we expect "always the latest version compatible with Asciidoctor v2.0.12?

  • If yes, then we should add a label to the image to specify the Asciidoctor PDF version
  • If no, then the minimalistic image will follow the lifecycle of asciidoctor itself, and will only serve as a basic building block for any user 's custom image and for the asciidoctor/box image

WDYT?

@mojavelinux
Copy link
Member

Yep, that's a good point. If you look at it that way, then yeah, Asciidoctor PDF should be in the box, not the base.

@reitzig
Copy link
Contributor Author

reitzig commented Mar 3, 2021

given the image asciidoctor/asciidoctor:2.0.12 , I expect to always have the latest Alpine Linux and latest Ruby version compatible with Asciidoctor v2.0.12.
But what about Asciidoctor PDF?

Good point. So: what's the version of box, anyway? 😅

That said, I guess we could, in one swoop, provide an official, minimal asciidoctor-pdf image, tagged according to its version? 🤔 But given that asciidoctor-pdf depends on asciidoctor, that seems silly as well.
I guess I find the prospect confusing that I need a 450MB image (local size) do build a plain PDF. But then, I can always extend asciidoctor/asciidoctor.

@mojavelinux
Copy link
Member

So: what's the version of box, anyway?

I think the box image should have its own versioning scheme.

@mojavelinux
Copy link
Member

I don't think there has to be only 2 images. We're just arguing that there shouldn't be just one. Maybe there needs to be a base, a mid-sized one that includes the major converters (PDF, EPUB3, reveal.js, etc), then the box that just provides it all.

@mojavelinux
Copy link
Member

mojavelinux commented Mar 3, 2021

It's always going to be a tradeoff though. With so many interchangable parts, there's always someone who wants a unique combination, like someone who wants to make HTML with diagrams and nothing else. These are not meant to be the only images ever made. They are about providing a foundation and something turnkey.

I don't really understand why we are quibbling over a few hundred MBs though. Does it really matter how big the image is (assuming what contains is actually useful and not just residual files from a build)?

@reitzig
Copy link
Contributor Author

reitzig commented Mar 3, 2021

Not sure if it makes sense to maintain it in this here repo.

The Dockerfile for asciidoctor/asciidoctor is <10 lines. Put it in project asciidoctor/asciidoctor and deploy from there.
Same for asciidoctor/asciidoctor-pdf or other, similar "just the one tool"-images.
That would be much clearer, and also remove the problem of when and how to trigger a new build for the asciidoctor/asciidoctor image (current proposal here would mean checking in a change with the new asciidoctor version, then creating a completely unrelated version tag for box, etc).

Finally, maintain "box" in this here repo, maybe in different flavors, with its own versioning schema.

If that makes sense to you, I'll remove main-minimal from this PR again and leave it to its original purpose. (forgot that the size issue was already merged, my bad)

FWIW, I don't understand the build target, anyway. 😅 If I surmise correctly, you make DockerHub build the image by triggering it via its API? Not even sure how that would work here, tagging multiple images from the same Dockerfile in separate repositories.
Alternative: docker login (IIRC, possible with tokens these days) + docher push.

@reitzig
Copy link
Contributor Author

reitzig commented Mar 3, 2021

I don't really understand why we are quibbling over a few hundred MBs though. Does it really matter how big the image is (assuming what contains is actually useful and not just residual files from a build)?

When used in CI (often, caching uncertain) or rarely but locally (re-pull every time), it can make a difference. Of course, you can argue that in such scenarios people should tailor their own images; but then there's not a whole lot of a point to a kitchen-sink image, isn't there? Provide a reasonable base, as you say, on top of which it's easy for people to install the gems they need. 🤷

That said, this MR comes from a completely unnecessary bloat from ~450MB to ~1.5GB which the MR effectively negates. @dduportal had the idea of solving #79 en passant -- I now think that was maybe not the best idea, as the issues are ultimately unrelated and have very different stakeholders, or so it seems.

So again, proposal: Let this here PR solve the size issue; discuss and implement different Docker image versions and their builds separately. (forgot that the size issue was already merged, my bad)

@mojavelinux
Copy link
Member

Put it in project asciidoctor/asciidoctor and deploy from there.

I don't like having Dockerfiles in the main repos. It mixes concerns.

@reitzig
Copy link
Contributor Author

reitzig commented Mar 4, 2021

I don't think adding another way of delivery to a repo mixes concerns too much; many projects include multiple install artifacts. On the other hand, as the above discussion and in particular the versioning conundrum shows, maintaining Docker images for different tools resp. "stacks" in the same repository can become rather impractical.

Anyway. This discussion is beyond my involvement in the project so I'll step out. Feel free to close the PR or adopt it, whatever your group decision is. Thanks!

@dduportal
Copy link
Contributor

Hello @reitzig , thanks a lot for all this inputs and for this contribution.
I understand that the time involvement here might be a concern.
I'll try, for the later reader, to answer the concerns raised in the discussion:

The build target vs. build in the DockerHub

Until recently, the DockerHub was a source of trust for end users, when a build is an "automated build". It helps the image consumers to put a certain trust that the downloaded image has been built from a certain Dockerfile (while non automated builds are always a risk has there is no proof it was built from a certain Dockerfile).
→ This is the reason why the "deployment" of the image is not a docker push from the CI, but a trigger to an automated build on the commit/tag that should be "deployed".

Why using a CI then? Because test. We need to test the generated image BEFORE pushing/deploying to the DockerHub. And the testing framework provided in the Docker Hub is quite limited (and really really slow: 1 hour to run he test harness of the asciidoctor image): https://docs.docker.com/docker-hub/builds/automated-testing/.
→ This is why we have a make build target, to propose a reproducible way for contributor that should build in the same way as the CI.

Alternative: docker login (IIRC, possible with tokens these days) + docher push.
You are making a valid point. After the backslash of the DockerHub API rate limit, and all the alternatives in the "container registry" area (GitHub, GCR, Quay.io, etc.), the "automated build" start to make less sense. The only "last check" should be to ensure that we can automate the update of the image description on a non automated build.
Doing so would simplify the whole build process and avoid confusion for contributors.

Managing multiple images from the same repo

The PR here is really nice, the only "challenging" item is the inclusion of asciidoctor-pdf` in the minimalistic image. But using a single (big) Dockerfile allows for managing the chained dependency and caching.

My initial request was to rename the images and map the renaming in the DockerHub (as you can specify a Dockerfile AND a target for each automated build):

  • asciidoctor/asciidoctor:2.0.15: minimalistic image, following the asciidoctor's gem lifecycle and version scheme. DockerHub is setup to build it from the target main-minimal from the Dockerfile.
  • asciidoctor/box:2.0.0: same as the current asciidoctor/docker-asciidoctor image, continues its version scheme (look at the GH release and the DockerHub tag), and is built on top asciidoctor/asciidoctor. DockerHub is setup to build it from the target main from the Dockerfile.

Asciidoctor PDF in the minimalistic image

I understand that, as a user, you are requesting for a lightweight image (or imageS) to run base asciidoctor commands for HTML and PDF export, without diagram, without epub support and without mathematical support.

It's a legit request, and it might be something to build outside of this PR of course (the subject is "adding a minimalistic image").

@dduportal
Copy link
Contributor

If it is OK for everyone, I'll merge this PR as it does not change anything on the final assets.
Because it is important to benefit from the time and effort that @reitzig put on these contributions.

A subsequent PR will come to adapt the minimalistic image to the new DockerHub image (setting the name and moving pdf back to the kitchen-sink image).

@dduportal
Copy link
Contributor

Thanks a lot @reitzig!

@dduportal dduportal merged commit 59c78ee into asciidoctor:main May 1, 2021
@dduportal dduportal added the chore Chore tasks (CI, automation, etc.) label May 1, 2021
@reitzig reitzig deleted the multi-stage-build branch April 12, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore tasks (CI, automation, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants