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

Migrate to an Alpine-based image & introduce a test suite #39

Merged
merged 31 commits into from
Oct 12, 2017
Merged

Migrate to an Alpine-based image & introduce a test suite #39

merged 31 commits into from
Oct 12, 2017

Conversation

dduportal
Copy link
Contributor

This Pull Requests follows up #24 .

It introduces:

  • A Test suite (using BATS ) to validate behaviour of the image

    • I don't cover ALL the use cases. This should be done on the fly ala BDD philosophy
    • We can see this test suite as an auto-generated "tech reference" of the image
  • Alpine Linux based image:

    • The final image weight around 470 Mb, which way less than the ~1,55 Gb of the original image
    • asciidoctor Alpine Linux Package is used, but not only
  • Asciidoctor-pdf is currently suffering (until next release) from a prawn dependency non wanted behaviour. This PR adds the short-term solution from @mojavelinux . More details here: can't modify frozen Array (RuntimeError) asciidoctor-pdf#772

  • "Plumbing sugar": gitignore, dockerignore, gitattributes (force unix eol)

  • What can be improved (WiP):

    • Using the Asciidoctor fixed version for the Alpine package
    • Using Travis CI (or any other free hosted CI) for building and testing, providing Pull Request feedback and confidence
  • Image can be tested from here (build time can be annoying): https://hub.docker.com/r/dduportal/docker-asciidoctor/ (it is a TEMPORARY docker image, do not use it outside testing !)

Now it's your turn to give feedbacks !

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Cleaning Docker image and tests

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Travis Init

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Travis wip

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Travis wip

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor Author

Travis CI build added. Feel free to stop me here if you do not use Travis for the asciidoctor organization. I can provide Jenkinsfile or Circle CI yml as well.

Build example here: https://travis-ci.org/dduportal/docker-asciidoctor/builds/213795084

FYI, a good CD pipeline could be:

  • Run Travis CI (or any other CI tools) that run the test suite (that takes care of locally building the image)
  • If test suite is passing, then, as a post-step, send a webhook to the DockerHub to launch the autobuild.

@mojavelinux
Copy link
Member

A Test suite

You are my hero already. This is the greatest gift to any project.

@mojavelinux
Copy link
Member

Asciidoctor-pdf is currently suffering (until next release) from a prawn dependency non wanted behaviour.

I'll have a new release out by the end of the weekend, hopefully even sooner.

@mojavelinux
Copy link
Member

mojavelinux commented Mar 23, 2017

Feel free to stop me here if you do not use Travis for the asciidoctor organization.

Stop you? I'm going to give you 🍾 🍾 🍾

(And yes, we mostly use Travis in Asciidoctor)

@mojavelinux
Copy link
Member

I'm going to note some things for the future, but I don't want them to hold up this PR, which is great:

  • Consider making a derivative image that adds fopub (aka the DocBook toolchain); we're trying to move away from that and, while there are still people that need it, it should be separate
  • I'm not convinced we need to install lazybones (though I do like that we're setting up sdkman)
  • Consider adding Asciidoctor Mathematical

.gitignore Outdated
@@ -0,0 +1 @@
tmp
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to make gitignore entries very specific so that they don't catch entries anywhere in the hierarchy. Thus:

/tmp/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll update it

Dockerfile Outdated
ENV ASCIIDOCTOR_VERSION "1.5.5"
ENV JAVA_HOME=/usr/lib/jvm/default-jvm \
PATH=${PATH}:${JAVA_HOME}/bin:/fopub/bin \
BACKENDS=/asciidoctor-backends \
Copy link
Member

Choose a reason for hiding this comment

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

Now that we've made separate gems for reveal.js and bespoke.js, I'm strongly considering dropping the backends from this image. If people disagree, I'm willing to change my mind.

Dockerfile Outdated
&& gem install --no-ri --no-rdoc asciidoctor-diagram \
&& gem install --no-ri --no-rdoc asciidoctor-epub3 --version 1.5.0.alpha.6 \
&& gem install --no-ri --no-rdoc asciidoctor-revealjs \
&& gem install --no-ri --no-rdoc rake \
&& gem install --no-ri --no-rdoc epubcheck --version 3.0.1 \
&& gem install --no-ri --no-rdoc kindlegen --version 3.0.1 \
Copy link
Member

Choose a reason for hiding this comment

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

We should move to kindlegen 3.0.3. I've verified it (on another project) and it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll update it

@mojavelinux mojavelinux changed the title Moving to an Alpine based image + introducing test suite (follow #24) Migrate to an Alpine-based image & introduce a test suite Mar 23, 2017
@bartoszmajsak
Copy link

That is awesome :) I will grab tmp image and give it a spin for generating @arquillian docs.

docker run -t --rm \
-v "${BATS_TEST_DIRNAME}":/documents/ \
"${DOCKER_IMAGE_NAME}" \
asciidoctor-pdf -D /documents/tmp /documents/fixtures/basic-example.adoc

Choose a reason for hiding this comment

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

I assume we won't change the content of HTML and PDF. Would it make sense to generate it and use to compare against what docker container produces?

If we see a value in it I can contribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @bartoszmajsak this totally make sense !

But let's go step by step (I like starting slowly and then improve):
If you're willing to do it now, you can easily fork my fork branch, start your own tests.
THen when you'll have finished, it will be easy to merge on my repo or in this, based on the status of this PR at that moment (power of decentralized tools).

Copy link

@bartoszmajsak bartoszmajsak Mar 23, 2017

Choose a reason for hiding this comment

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

Sure, just wanted to hear your thoughts. It's not for this PR per se, but I will work on it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, no problem. Thanks for this, this is a great idea !

Dockerfile Outdated
rubygem-nokogiri \
jpeg \
jpeg-dev \
openjdk8 \
Copy link
Member

Choose a reason for hiding this comment

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

openjdk8-jre-base should be sufficient here. openjdk8 is complete JDK including dependencies for desktop apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, this come from fopub tool: https://github.com/asciidoctor/asciidoctor-fopub#prerequisites . But given @mojavelinux comment below (#39 (comment) ) , this would be a great move if we got out the jdk to the jre (even not using java at all in this image)

Dockerfile Outdated
jpeg-dev \
openjdk8 \
patch \
python \
Copy link
Member

Choose a reason for hiding this comment

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

This package was renamed to python2 (python2-dev).

Copy link
Contributor Author

@dduportal dduportal Mar 23, 2017

Choose a reason for hiding this comment

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

👍 I'll update it

Dockerfile Outdated
&& mkdir "${BACKENDS}" \
&& (curl -LkSs https://api.github.com/repos/asciidoctor/asciidoctor-backends/tarball | tar xfz - -C "${BACKENDS}" --strip-components=1) \
&& ln -s /usr/bin/easy_install-2.7 /usr/local/bin/easy_install \
&& easy_install 'blockdiag[pdf]' \
Copy link
Member

Choose a reason for hiding this comment

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

easy_install is ancient crap that should not be used anymore, use pip instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll update it

@dduportal
Copy link
Contributor Author

dduportal commented Mar 23, 2017

Based on this first set of feedbacks (Thanks you all !), here is the task list for this PR (I'll take it updated on the fly):

  • .gitignore more specific for the tmp dir (and/or use the BATS_TMP_DIR instead)
  • Upgrade kindlegen gem
  • Remove Backends (at least for this PRn, and see if people don't like it)
  • Remove fopub and open an issue for tracking creation of a derivative image (using docker-compose to combine with this one ?) + remove java instead of needed
  • python: use the right packages
  • pip instead of easy_install
  • Remove lazybone but keep sdkman installed (not very heavy)
  • Install Asciidoctor mathematical https://github.com/asciidoctor/asciidoctor-mathematical#installation

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Python 2 package dev cleanup

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor Author

Hello, thanks for the notice @jirutka !

It has been updated to alpine 3.6, with Gems updates also (asciidoctor pdf and asciidocotr alpine packer to r1).

Ping @mojavelinux if it is ok for you to merge ?

@jirutka
Copy link
Member

jirutka commented Oct 2, 2017

@mojavelinux Why is this still not merged?

@madpipeline
Copy link

@jirutka I've been wondering the same thing for months

@wimdeblauwe
Copy link

I would love to see this happen as well. Seems the current version on dockerhub is still using asciidoctor 1.5.5 (https://hub.docker.com/r/dduportal/docker-asciidoctor/~/dockerfile/).

@wimdeblauwe
Copy link

Just a heads up that I tried the image but I get the following error if I want to use asciidoctor diagram:

 asciidoctor-diagram: ERROR: Failed to generate image: Could not find Java executable

I used the latest version of the code in this PR + I replaced version 3.6 with edge for the Linux and set asciidoctor version to 1.5.6.1-r0

@jirutka
Copy link
Member

jirutka commented Oct 4, 2017

I get the following error if I want to use asciidoctor diagram:

asciidoctor-diagram: ERROR: Failed to generate image: Could not find Java executable

Well, that’s no surprising, there’s no openjdk-jre-base package in the dockerfile. However, why does it need Java? o.O

I replaced version 3.6 with edge

That’s not a good idea, edge is unstable.

@dduportal
Copy link
Contributor Author

Hello @wimdeblauwe @jirutka! Thanks for your feedbacks!
About the missing java in the image, it comes from @mojavelinux
comment at the beginning of the current PR: #39 (comment)
=> One strategy would be having a dedicated tag or image with java embedded to cover this. The tradeoff here is that a lot of the user might not need the java, but are interested in a lightweight image. But still, in term of usability, the end-user should not have to care about this.
=> Maybe a composed image could be a solution, with the new https://docs.docker.com/engine/userguide/eng-image/multistage-build/ .

Ping @mojavelinux we need you or a maintainer to merge this in order to iterate. I am willing to help, even maintaining if you need help or time.

@jirutka
Copy link
Member

jirutka commented Oct 6, 2017

@dduportal I pinged @mojavelinux on Twitter and asked him to give you a write access. It really sucks to wait months for response/approval.

@dduportal
Copy link
Contributor Author

Thanks @jirutka . I would not say that "it sucks" because I already had issues to cover all requests on my very tiny personnal projects, so I can understand the overloading for @mojavelinux .

By the way, willing to maintain will help I hope :)

@mojavelinux
Copy link
Member

It's definitely time (maybe even past time) to make this switch. We're all ready for it and other requests are getting backed up because this isn't merged yet. While I could merge, I would only just be pressing a button, not testing it. Therefore, I'm granting @dduportal admin rights on the repository. @dduportal you can choose to maintain it or grant permission to another trust member of the community willing to step forward. You've demonstrated your skill set in this issue and I trust you to be the one who merges it. We all appreciate your contribution. I want to empower you to continue this work.

@dduportal
Copy link
Contributor Author

Thanks @mojavelinux for this trust.

Just tested again and it is working on my Mac, on Travis, and on my remote Ubuntu server.

Now merging and taking care of any issue question from now.

@dduportal dduportal merged commit ed77567 into asciidoctor:master Oct 12, 2017
@dduportal dduportal deleted the alpine branch October 12, 2017 20:35
@dduportal dduportal mentioned this pull request Oct 12, 2017
@mojavelinux
Copy link
Member

👏

rassie added a commit to rassie/docker-asciidoctor that referenced this pull request Oct 13, 2017
PR asciidoctor#39 which moved the image to alpine has also thrown out make, which is quite handy for more complicated build scenarios.
@rassie rassie mentioned this pull request Oct 13, 2017
rassie added a commit to rassie/docker-asciidoctor that referenced this pull request Oct 16, 2017
PR asciidoctor#39 which moved the image to alpine has also thrown out make, which is quite handy for more complicated build scenarios.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants