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

Travis CI build does not fail on Asciidoctor warning #1491

Closed
HonkingGoose opened this issue Aug 17, 2020 · 7 comments
Closed

Travis CI build does not fail on Asciidoctor warning #1491

HonkingGoose opened this issue Aug 17, 2020 · 7 comments

Comments

@HonkingGoose
Copy link
Contributor

HonkingGoose commented Aug 17, 2020

Executive summary/TL:DR:

The current rake script cannot fail the build on Asciidoctor warnings/errors. We cannot use the version of Asciidoctor we're currently using, because that does not have the necessary flag (--failure-level=LEVEL. This flag is available from Asciidoctor 1.5.7.0, but using >1.5.7.0 version without further changes to the book will break cross-reference links.

Proposed way to resolve this:

  • Update to Asciidoctor 2.0.10 in Switch to asciidoctor 2.0.10 #1373
  • Run Asciidoctor migration script from @jnavilla.
  • Speed up build error feedback time by using the fast html build first. Only build the epub, mobi and pdf files if the html build passes.

Which version of the book is affected?

Problem is with the Travis CI, source code on master.

Describe the bug:

The Travis CI build is passing, while the output of build log contains warnings from Asciidoctor build

The Travis CI build should fail when Asciidoctor logs a warning when running bundle exec rake book:build.

Steps to reproduce:

  1. Open pull request with a Asciidoctor style error, like missing a newline before a unordered list (Example: Create section: branch renaming #1476 pull request).
  2. Travis CI runs bundle exec rake book:build.
  3. Asciidoctor logs a WARNING.
  4. Travis CI passes.
  5. GitHub lets you merge bad stuff.

Expected behavior:

  1. Open pull request with a Asciidoctor style error, like missing a newline before a unordered list (Example: Create section: branch renaming #1476 pull request).
  2. Travis CI runs bundle exec rake book:build.
  3. Asciidoctor logs a WARNING.
  4. Warning is caught by the Travis CI in some way.
  5. Travis CI fails.
  6. GitHub stop your from merging bad stuff because the test is failing.

Additional context:

I noticed that pull #1490 is fixing something that should not have been allowed past the Travis CI build.

Example of a build which has a Asciidoctor warning but a passing Travis CI build:
https://travis-ci.org/github/progit/progit2/builds/718311991
Line 319 has the first warning (it issues the same warning 3 times):

asciidoctor: WARNING: book/03-git-branching/sections/branch-management.asc: line 80: invalid style for listing block: CAUTION

My research into a possible fix:

I found out that Asciidoctor gained the ability to error out on a given failure level (asciidoctor/asciidoctor#2003):

From Asciidoctor manual about processing

--failure-level=LEVEL
The minimum logging level that triggers a non-zero exit code (failure). If this option is not set (default: FATAL), the program exits with exit code zero even if warnings or errors have been logged.

From the Asciidoctor user manual, handle missing or undefined attributes

If you want the processor to fail when the document contains a missing attribute, set the attribute-missing attribute to warn and pass the --failure-level=WARN option to the processor.

From the Asciidoctor user manual Appendix D: Application messages

ERROR Errors do not stop conversion, but the output document will almost certainly be wrong.
FAILURE Failures are fatal; no output document will be produced.
WARNING Warnings do not stop conversion, but they indicate possible problems, and the output may not be what you were expecting.


Attempt at fixing this:

WARNINGS:

It might be possible to fix this, by letting Asciidoctor fail when the .html build of the book contains errors.
⚠️ Do not change the params variable in the rake file, as that applies to all 4 of the builds.
🐛 The asciidoctor-pdf program may not use the logger: asciidoctor/asciidoctor#2003 (comment) and may crash when using --failure-level=WARN.


I would first try changing this line into:

bundle exec asciidoctor --failure-level=WARN --attribute revnumber='#{version_string}' --attribute revdate='#{date_string} -a data-uri progit.asc

and see where that gets us.

I couldn't find a --failure-level=ERROR in the official documentation, this was mentioned here though: asciidoctor/asciidoctor#2003 (comment). So it might exist, or it might not exist.


Alternatively, you can also close this issue as wont-fix. That's also OK. I wanted to flag the issue, so that the maintainer can decide on what to do. 👍

@HonkingGoose
Copy link
Contributor Author

HonkingGoose commented Aug 18, 2020

Well after some noodling around, I figured out that the version of Asciidoctor we're using (1.5.6.2) does not have the necessary flag --failure-level=.

From the Asciidoctor changelog for 1.5.7:

add option to CLI to force non-zero exit code if specified logging level is reached (#2003, PR #2674)

And from the 1.5.7 release summary:

The most significant change in this release is that all warning and error messages are now routed through a logger (finally!). Many of the messages also include context about the source location (file, dir, path, lineno), which can be useful for integrations and tooling. You can even force the CLI to exit with a non-successful exit code if the messages reach the threshold specified using the --failure-level option. When using the API, you can feed your own logger to the LoggerManager to capture, route, or observe the messages.

So I'm going to try updating the Asciidoctor version to that version and see where that gets me.


Well that was a bust, I tried some more variations on the same them, but cannot get the build to fail with the currenct rake script setup. The logging output from Asciidoctor does not make it to any part of Travis CI, or to a part of the rake script that can catch the issued warnings/errors and fail the build.

I also found this issue: Processor doesn't fail when it should: asciidoctor/asciidoctor#3078

EDIT: We do not need a custom logger.
So the solution to this problem would probably be to put more logic into the script, so that we fail the build when there is a error. This would probably mean writing a custom error handler in the rake script.

As a side note: using Asciidoctor 1.5.7.1 leads to a lot of broken references in the book, but that issue was already found here: #1354 (comment).

using asciidoctor version > 1.5.6.2 makes cross-reference links not work

@slonopotamus
Copy link
Contributor

There's #1373 that aims at upgrading Asciidoctor to 2.x. If/when it is merged, you can just use --failure-level=.

@HonkingGoose
Copy link
Contributor Author

HonkingGoose commented Aug 25, 2020

Hi @slonopotamus, thanks for responding!

There's #1373 that aims at upgrading Asciidoctor to 2.x. If/when it is merged, you can just use --failure-level=.

I'm not sure if just using the failure-level= option will actually fail the build when there is a error. I'm under the impression that some kind of custom logger is needed, one that catches the emitted error and gives the "fail build" command to the CI.
EDIT: We do not need a custom logger.

I've tried to get the build failing on my fork at https://github.com/HonkingGoose/progit2/pull/4, when just using the --failure-level option. The Asciidoctor console output issues a warning, but Travis CI will still pass the build. I have tried multiple variants, but could not get a failing Travis CI build out of it.

Maybe you can take a look at my attempts and maybe see what I'm missing here? Or do you maybe have a working example of the option being used with Travis CI/GitHub Actions, or something similar?

@HonkingGoose
Copy link
Contributor Author

HonkingGoose commented Aug 25, 2020

Hi @ben, I managed to get a simple proof of concept of my proposed changes working on my fork at https://github.com/HonkingGoose/progit2/pull/4.

I will need to do some more work, but I wanted to show you the proof of concept and get your feedback on it.

The proper full fix can only be merged when the version of Asciidoctor that we're using is 1.5.7.0 or newer, and the ruby gems can all be installed into Travis CI again (#1496).

Do you want me to continue working on this issue? Would you consider a pull request from me that fixes the issue fully, merge-able?

Greetings HonkingGoose

@ben
Copy link
Member

ben commented Aug 28, 2020

I would absolutely consider a fully-formed PR that fixes all of our build issues. 😁

@HonkingGoose
Copy link
Contributor Author

Cool! I'll continue on working on this then! 🎉 🚀

I really want a build that gives me more confidence that I'm not messing things up for you! Catching errors early is more helpful than afterwards. 😄

@HonkingGoose
Copy link
Contributor Author

Closing this as @jnavila improved our Travis CI builds by making a HTML and EPUB checker with PR: #1564

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

No branches or pull requests

3 participants