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 bugs about the exit code #262

Merged
merged 3 commits into from
Nov 7, 2024
Merged

Fix bugs about the exit code #262

merged 3 commits into from
Nov 7, 2024

Conversation

YDX-2147483647
Copy link
Contributor

@YDX-2147483647 YDX-2147483647 commented Nov 5, 2024

No doc update required.

@@ -29,19 +36,17 @@ fi

# Execute lychee
eval lychee ${FORMAT} --output ${LYCHEE_TMP} ${ARGS}
exit_code=$?
LYCHEE_EXIT_CODE=$?
Copy link
Contributor Author

@YDX-2147483647 YDX-2147483647 Nov 5, 2024

Choose a reason for hiding this comment

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

I change it to uppercase to imply the variable is immutable and does not describe the exit code of entrypoint.sh.

@YDX-2147483647 YDX-2147483647 changed the title fix: Make fail: false effective even when failIfEmpty: true Fix bugs about the exit code Nov 5, 2024
@YDX-2147483647 YDX-2147483647 marked this pull request as ready for review November 5, 2024 17:24
@YDX-2147483647 YDX-2147483647 marked this pull request as draft November 5, 2024 17:26
This commit also makes sure `outputs.exit_code` is “The exit code returned from Lychee”. `failIfEmpty` no longer changes it to `1`.

Relevant docs:
- [Setting exit codes for actions - GitHub Docs](https://docs.github.com/en/actions/sharing-automations/creating-actions/setting-exit-codes-for-actions)
- [exit - POSIX Programmer's Manual](https://manned.org/exit.1posix)

Relates to lycheeverse#86, lycheeverse#128, lycheeverse#145, lycheeverse#245, and lycheeverse#251.
The previous expression always gives `false`.
Both `env.exit_code` and `env.lychee_exit_code` are `null`, probably since the docker→composite refactor lycheeverse#128. When GitHub evaluates the expression, it finds the types do not match, and coerces them to number, namely, `null` → `0`.

See [Evaluate expressions in workflows and actions - GitHub Docs](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#operators).

Relates to lycheeverse#253.
@YDX-2147483647
Copy link
Contributor Author

YDX-2147483647 commented Nov 5, 2024

Now it passes the test.yml added in #251, at least locally.

$ nix-env -iA nixpkgs.act
$ act -W .github/workflows/test.yml  # with medium size image

Gitpod

By the way, as lychee is a rust project, I bet those two bugs would never happen if variables are immutable by default and std::env::{var, var_os} gives Result/Option.

@YDX-2147483647 YDX-2147483647 marked this pull request as ready for review November 5, 2024 17:50
Edited from lycheeverse#251.

Co-Authored-By: Sebastiaan Speck <[email protected]>
@YDX-2147483647
Copy link
Contributor Author

YDX-2147483647 commented Nov 6, 2024

#251 (comment)

merge the changes of this PR (#251) into your branch (#262)

So I've just done it. I git cherry-pick and add the tests described in #251 (comment).

(✅ = already covered, 🌟 = this PR)

  • If fail: false and failIfEmpty is not set, don't fail if the status code is not 0 🌟 but fail if no links were found 🌟.
  • If fail is not set, and failIfEmpty: false, fail if the status code is not 0 🌟 but don't fail if there are no links ✅.
  • If none is set, use the defaults: fail if the status code is not 0 🌟 or no links were found ✅.

@mre mre merged commit 988c4c1 into lycheeverse:master Nov 7, 2024
3 checks passed
@mre
Copy link
Member

mre commented Nov 7, 2024

Thanks for fixing and for documenting the process so neatly. Very well done! 🌟

@@ -23,7 +24,7 @@ jobs:
GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}

- name: Create Issue From File
if: env.exit_code != 0
if: ${{ steps.lychee.outputs.exit_code }} != 0
Copy link
Member

@mre mre Nov 7, 2024

Choose a reason for hiding this comment

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

@YDX-2147483647, this seems to have led to a false-positive issue creation here: #264

Note that there were no issues with the check run, but it still created the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry… I guess ${{ steps.lychee.outputs.exit_code }} != 0 evaluates to "0 != 0" (a string), and becomes true when GitHub coerces its type.

So it should be if: steps.lychee.outputs.exit_code != 0 or if: ${{ steps.lychee.outputs.exit_code != 0 }}. Not sure, however. I'll test it in my own repo later (in one or two days).

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 have just tested it, and it works for both success and failure. Please see #265.

YDX-2147483647 added a commit to YDX-2147483647/lychee-action that referenced this pull request Nov 8, 2024
`${{ steps.lychee.outputs.exit_code }} != 0` evaluates to `"0 != 0"` (a string), and becomes `true` when GitHub coerces its type.

Relates-to: lycheeverse#262
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.

3 participants