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

The lychee exit status doesn't seem to be be propagated correctly through the action (by default) #71

Closed
dkarlovi opened this issue Jan 26, 2022 · 5 comments

Comments

@dkarlovi
Copy link

I have the action currently setup like this:

            -
                name: Link Checker
                uses: lycheeverse/lychee-action@master
                with:
                    args: --verbose --offline --base ./tests/functional/init/public ./tests/functional/init/public

It gives me this output:

+ LYCHEE_TMP=/tmp/lychee/out.md
+ GITHUB_WORKFLOW_URL='https://github.com/sigwinhq/yassg/actions/runs/1750502305?check_suite_focus=true'
++ dirname /tmp/lychee/out.md
+ mkdir -p /tmp/lychee
+ ARGS='--verbose --offline --base ./tests/functional/init/public ./tests/functional/init/public'
+ FORMAT=
+ [[ --verbose --offline --base ./tests/functional/init/public ./tests/functional/init/public =~ --format  ]]
+ FORMAT='--format markdown'
+ lychee --format markdown --output /tmp/lychee/out.md --verbose --offline --base ./tests/functional/init/public ./tests/functional/init/public
Error: Failed to read file: `./tests/functional/init/public`, reason: Is a directory (os error 21)
+ exit_code=1
+ '[' 1 -ne 0 ']'
++ dirname -- lychee/out.md
+ mkdir -p lychee
+ cat /tmp/lychee/out.md
cat: /tmp/lychee/out.md: No such file or directory
+ echo '[Full Github Actions output](https://github.com/sigwinhq/yassg/actions/runs/1750502305?check_suite_focus=true)'
+ cat /tmp/lychee/out.md
cat: /tmp/lychee/out.md: No such file or directory

+ echo

+ echo ::set-output name=exit_code::1
+ '[' false = true ']'

But the action passed? Seems the exit code from any of the commands didn't get propagated correctly.

@dkarlovi
Copy link
Author

Reading through the docs, seems this is by design, but IMO it's not a good default to not fail.

@mre
Copy link
Member

mre commented Jan 26, 2022

Sounds like you found https://github.com/lycheeverse/lychee-action#alternative-approach already.

But just in general, the thought process was that lychee was used as a step within a bigger "linting" pipeline and we didn't want to fail the entire pipeline because of a broken link. Now the question is if broken links are critical for the success of an entire action run.
We have some bigger projects using lychee that would break if we change that, but it's not too late to discuss. Would like to see some strong arguments in favor of failing the pipeline, though, because this would be a major breaking change and making it fail is as simple as adding fail: true.

@dkarlovi
Copy link
Author

@mre thanks for the explanation.

From my POV, the GH action is basically "lychee in a box", it should align completely with what the app does by default. If you add a new step / job to a CI, typically it failing short circuits the entire thing by default because that's how the underlying tools work, even doing ls no-such-file will fail the build.

This is somewhat outlined in the Principle of least astonishment where things should happen how most users expect it to happen. A CI job not failing when the underlying tool fails is not following that.

making it fail is as simple as adding fail: true.

I'd argue that making it not fail is as simple as adding fail: false. :)

Again, the GH action should be "convenience around the tool" and should ideally by default behave exactly like the tool. Any additional features the action provides should be strictly opt-in for users who want more.

@dkarlovi dkarlovi changed the title The lychee exit status doesn't seem to be be propagated correctly through the action The lychee exit status doesn't seem to be be propagated correctly through the action (by default) Jan 27, 2022
@mre
Copy link
Member

mre commented Jan 27, 2022

I agree. If there are no objections I'm all for changing it.
@lycheeverse/contrib fyi

wolf99 added a commit to exercism/problem-specifications that referenced this issue Nov 4, 2022
wolf99 added a commit to exercism/problem-specifications that referenced this issue May 28, 2023
kytrinyx pushed a commit to exercism/problem-specifications that referenced this issue Jun 8, 2023
* Add link checking for markdown in CI

Fixes #2139

* Require HTTPS

* Update Lychee action

* Make flow fail on error

Per: lycheeverse/lychee-action#71

* Update lychee action to 1.8.0

* Add .lycheeignore
@mre mre closed this as completed in 30414e4 Apr 25, 2024
@mre
Copy link
Member

mre commented Apr 25, 2024

This is fixed in master now. So you can already use it with lychee-action@master. It will be released with v2, which I'm planning to ship in a few weeks after the dust has settled a bit and people got a chance to update their pipelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants