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

Correct code snippet in README #135

Closed
wants to merge 2 commits into from
Closed

Conversation

Snuggle
Copy link

@Snuggle Snuggle commented Jun 25, 2022

Hello!

I noticed that after copy/pasting the GitHub action snippet, this if check does not work.

I've replaced it with if: failure() which seems to actually work now. Please feel free to make any edits needed to this pull request!

@mre
Copy link
Member

mre commented Jul 1, 2022

Weird, I thought we fixed that for good in #102. 🤔
failure() sounds more like it unconditionally triggers a failure, but I haven't looked it up yet.

@polarathene
Copy link

polarathene commented Jul 3, 2022

TL;DR:

  • This is semi-correct, especially considering the 2.0 plan to flip the default for the fail input (that input doesn't seem worth keeping as described below).
  • An improvement would be to add an extra condition to check for the lychee-action step status specifically, just in-case the workflow implemented by the user has earlier steps that could fail the job too (see below example).

failure() sounds more like it unconditionally triggers a failure, but I haven't looked it up yet.

It's an expression function, specifically for this kinda of handling.

Expand for verbose details

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-status-check-functions
https://docs.github.com/en/actions/learn-github-actions/expressions#failure

Returns true when any previous step of a job fails. If you have a chain of dependent jobs, failure() returns true if any ancestor job fails.

failure with conditions
You can include extra conditions for a step to run after a failure,
but you must still include failure() to override the default status check of success() that is automatically applied to if conditions that don't contain a status check function.

The linked docs have an example for "failure with conditions" that shows that you can check if a specific step failed:

if: ${{ failure() && steps.lychee-action.conclusion == 'failure' }}

Alternatively one can use continue-on-error (step context docs):

When a continue-on-error step fails, the outcome is failure, but the final conclusion is success.

step.continue-on-error:

  • Prevents a job from failing when a step fails.
  • Set to true to allow a job to pass when this step fails.

Which AFAIK could be used with the lychee-action step, inversely providing the same purpose of the fail input for this action? (and aligned instead of inverse, with the planned 2.0 change for fail input):

  • Fail action on broken links
  • Fail entire pipeline on error (i.e. when lychee exit code is not 0)

# Pass lychee exit code to next step
echo ::set-output name=exit_code::$exit_code
# If `fail` is set to `true`, propagate the real exit value to the workflow
# runner. This will cause the pipeline to fail on exit != 0.
if [ "$INPUT_FAIL" = true ] ; then
exit ${exit_code}
fi

That is... by default always exiting the action with the status code from the lychee command (as if lychee-action had fail: true input as default, like it will in 2.0).

To allow failure for the step and continue the job, users opt-in by adding continue-on-error: true instead, which is more descriptive of what to expect and consistent with other actions?

Likewise, instead of continue-on-error: true on the lychee-action step, users can define later steps with the failure() conditional to only run those steps in response to a prior step failing (which as shown above can specifically reference status via the conclusion of a step).


step.continue-on-error: true vs step.if: failure():

  • step-a.continue-on-error: true => step-a.outcome == failure && step-a.conclusion == success.
    • step-b will run even if step-a failed.
  • step-a.outcome == failure is valid for step-b.if: failure(), while step-a.conclusion may differ.
    • step-b will only run if step-a failed (useful for conditionally reporting an issue).

README.md Outdated Show resolved Hide resolved
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
@polarathene
Copy link

this if check does not work.

I have since realized that the exit_code output is internal to lychee-action, it's not actually making it available to other steps.


I'm also on the fence about failure() being appropriate, as the intent here is for opening an issue when exit_code == 2, not other failures which you might want to be notified about (GH at least emails me when a workflow job fails AFAIK, and will visualize it as a failure).

In this scenario, the outcome you want is to create an issue when lychee-action finds link errors, if you don't have any other outcomes to support (eg: that is the sole purpose of the job and it's running on a schedule), then it should not be logged/reported as a failure to the CI (the issue creation is handling that responsibility), but successful as the failure is expected to be found and delegated/reported, while still being successful if no errors are found.

Whereas with a lint task on a PR activity, you're interested in the check-suite status. A failure for a job there is more meaningful, and a simple way of showing what tasks failed.

@mre
Copy link
Member

mre commented Jul 11, 2022

If someone finds the time, I'd be thankful for a review of #145, which is another attempt to fix this issue.

@polarathene
Copy link

I will review later today. I should be able to spare some more time later this week to make some contributions too hopefully.

@mre
Copy link
Member

mre commented Jul 12, 2022

#145 has landed, which should hopefully fix that issue. It was apparently broken since we moved to a composite action. @Snuggle can you give master another try?

@mre
Copy link
Member

mre commented Jul 27, 2022

Gonna close this as fixed. Thanks to you both for your contributions. 😊 Please don't hesitate to open more issues or pull requests.

@mre mre closed this Jul 27, 2022
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