Detecting pipe failures prevents result reporting

LukeMondy commented 4 years ago

Having pipefail set in the entrypoint.sh stops failures being reported if html5validator does actually return a non-zero code.

You can see this in the logs from the action-test here: https://github.com/Cyb3r-Jak3/html5validator-action/runs/936410945?check_suite_focus=true

For the "Plain test" step, we see this output:

Running Validator
+ BuildARGS=
+ uses ''
+ '[' -n '' ']'
+ usesBoolean false
+ '[' -n false ']'
+ '[' false = true ']'
+ uses ''
+ '[' -n '' ']'
+ tee log.log
+ html5validator --root tests/valid/ --log WARNING
+ result=0
0
+ usesBoolean true
+ '[' -n true ']'
+ '[' true = true ']'
+ echo 0
+ echo ::set-output name=result::0

But for the "Check failure" step, we see this:

+ BuildARGS=
+ uses ''
+ '[' -n '' ']'
+ usesBoolean true
+ '[' -n true ']'
+ '[' true = true ']'
+ BuildARGS+=' --also-check-css'
+ uses -lll
+ '[' -n -lll ']'
+ BuildARGS+=' -lll'
+ html5validator --root tests/invalid/ --log INFO --also-check-css -lll
+ tee log.log
Running Validator
INFO:html5validator.cli:Files to validate: 
  tests/invalid/index.html
  tests/invalid/style.css
INFO:html5validator.cli:Number of files: 2
ERROR:html5validator.validator:['"file:/github/workspace/tests/invalid/index.html":0.1-0.6: error: Start tag seen without seeing a doctype first. Expected "<!DOCTYPE html>".', '"file:/github/workspace/tests/invalid

Note that the lines after + html5validator --root tests/valid/ --log WARNING in the "Plain test" don't get reported in the "Check Failure" case.

Doing a quick test with this PR seemed to give the correct behavior.

Cyb3r-Jak3 commented 4 years ago

Hello @LukeMondy , What is the behavior that you are looking for? Your pull request is going to cause a regression back to #4 as without the set -o pipefail the line html5validator --root "${INPUT_ROOT}" --log "${INPUT_LOG_LEVEL}" ${BuildARGS} |& tee log.log will alawys return a 0 exit code even if html5validator fails.

LukeMondy commented 4 years ago

Ah, sorry, I didn't look into the closed issues.

I'm looking to use the output result in later stages, to check to see what the return code is, e.g.: https://github.com/Cyb3r-Jak3/html5validator-action/blob/6a7177075df0b8d88b6930125f5c54f1e41e67ae/.github/workflows/action-test.yml#L43

When the html5validator finds an issue, it returns a non-zero exit code. In the current state of the code, because of pipefail being set, this line won't get run: https://github.com/Cyb3r-Jak3/html5validator-action/blob/6a7177075df0b8d88b6930125f5c54f1e41e67ae/entrypoint.sh#L33

Which means later on, when I check the result variable, it's unset. It also means more generally that the result variable will either be 0 or unset, never any other value.

When I remove the pipefail option, the result correctly reports an error code (such as 255).

I do see the issue though - the action may report passing when the html5validator didn't actually return a 0 exit code. To fix that, we could add:

exit $result

after this line: https://github.com/Cyb3r-Jak3/html5validator-action/blob/6a7177075df0b8d88b6930125f5c54f1e41e67ae/entrypoint.sh#L33

That would still allow the job to fail, but still report the return code in the result output.

Cyb3r-Jak3 commented 4 years ago

I follow and that makes sense to make sure that this action does work in a better context. I have gone ahead and created those changes (Pull Request #9) and the behavior you are looking for now exists in version v0.4.2.

I am going to close this pull request and once you get a chance to test please let me know.

Cyb3r-Jak3 / html5validator-action

Detecting pipe failures prevents result reporting #8