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

Fix #44: Re-Enabling Asciidoctor-diagram usage #46

Merged
merged 4 commits into from
Oct 21, 2017
Merged

Fix #44: Re-Enabling Asciidoctor-diagram usage #46

merged 4 commits into from
Oct 21, 2017

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented Oct 15, 2017

The Pull Request #39 broke the usage of Asciidoctor-diagram.

This pull request aims to fix it, by introducing the following:

  • A sample from asciidoctor-diagram doesn't work on alpine based image #44 has been added to validate usage and provide non-regression in future upgrades
  • 3 test cases have been introduced to cover the tools that where missing
  • Install OpenJDK8-JRE (not the full JDK: I might miss something but we just want to execute with Java)
  • Install Graphviz, required for generating diagrams

[EDIT]
The image is built (and updated) on the DockerHub and can be pulled without building it:

docker pull dduportal/docker-asciidoctor:fix-adoc-diagram

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
@mojavelinux
Copy link
Member

We probably want a slightly more complex example to make sure we put some weight on graphviz to verify it works. Perhaps @pepijnve can advise. @pepijnve do you have a go-to example that you use (perhaps from the test suite) that thoroughly tests the graphviz setup?

I'm pretty sure all we need is a JRE.

… clean file avoiding errors.

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

The build on my own TravisCI was broken due to the "files owned by root" when cleaning the test cases: https://travis-ci.org/dduportal/docker-asciidoctor/builds/288124855

Just added a fix on the test harness suite to avoid breaking any kind of CI, by using "docker for everything" pattern.

@dduportal
Copy link
Contributor Author

dduportal commented Oct 20, 2017

Hello @mojavelinux!

I just want to be sure I get the goal:

Should we:

@mojavelinux
Copy link
Member

I'm fine with moving forward and filing a separate issue for strong tests. I just mentioned it because I'm concerned this is going to keep biting us if we don't test it more thoroughly.

@dduportal dduportal merged commit 6f28c62 into asciidoctor:master Oct 21, 2017
@dduportal
Copy link
Contributor Author

This Pull Request have been merged.

@mojavelinux, the issue #48 has been opened to keep track of the graphviz test weight.
Thanks for your quick feedback and review!

@dduportal dduportal deleted the fix-adoc-diagram branch October 21, 2017 10:53
@mojavelinux
Copy link
Member

Thanks @dduportal!

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.

2 participants