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

Fail pipeline on error by default (fixes #71) #85

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Fail pipeline on error by default (fixes #71) #85

merged 1 commit into from
Apr 25, 2024

Conversation

mre
Copy link
Member

@mre mre commented Mar 1, 2022

Previously, a pipeline would fail if fail was set to true. In the beginning I wasn't sure if this should be the default, so this is an opt-in. Feedback from users showed that not failing the pipeline on broken links is somewhat unexpected (compare #71). Therefore I'd like to change this and make failing the default.

Since this a potentially breaking change, I consider releasing this as part of v2.0.0.

@dkarlovi fyi

@polarathene
Copy link

From #71 (comment)

  1. 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.

    The workflow maintainer should use continue-on-error: true for lychee-action in this case. They can still check the status of lychee-action specifically with steps.lychee.outcome.

    Neither exit_status output, nor fail input for lychee-action seem necessary for this action. The purpose they serve is already supported with official workflow syntax that applies for all steps generically.

  2. Would like to see some strong arguments in favor of failing the pipeline

    Steps can run only when a failure occurs, such as creating an issue report.

    That does not require the pipeline/job to fail however. continue-on-error: true will allow lychee-action to always be "successful" (steps.lychee.conclusion), but steps that are interested can still know if the step actually failed (steps.lychee.outcome).

If you'd like to know more, I have gone into more detail (with references to official GH action docs) in a related PR discussion.


I think it would be better to drop both outputs.exit_status + inputs.fail for 2.0, and raise awareness via documentation of their equivalents:

  • steps.lychee.continue-on-error: true:
    • Don't fail a job early when the step fails (sets: steps.lychee.conclusion = 'success').
    • Step retains failure status (steps.lychee.outcome = 'failure').
  • steps.other-step.if: ${{ failure() && steps.lychee.outcome == 'failure' }}:
    • steps.other-step is only run when steps.lychee step fails.

@mre
Copy link
Member Author

mre commented Apr 25, 2024

I think it would be better to drop both outputs.exit_status + inputs.fail for 2.0, and raise awareness via documentation of their equivalents.

I see your point, but I'm not fully convinced. While the methods you've described are correct, they may not be straightforward for everyone. Not all users might take the time to read through the documentation to find that specific note. Having a clear argument in lychee-action makes it easier to find and use.

Also, we already have a fail parameter, which makes it easy to adjust. Users can just set fail: false if the update causes any problems. We'll make sure to point this out in the release notes.

I think it's important to go ahead with this update, since it has been pending for a while. Let's make this small improvement now. We can always come back to this topic in version 3, making any adjustments easier then.

@mre mre merged commit 30414e4 into master Apr 25, 2024
4 checks passed
@mre mre deleted the fail-true branch April 25, 2024 12:59
weiji14 added a commit to GenericMappingTools/pygmt that referenced this pull request Oct 15, 2024
seisman pushed a commit to GenericMappingTools/pygmt that referenced this pull request Oct 16, 2024
* Build(deps): Bump lycheeverse/lychee-action from 1.10.0 to 2.0.2

Bumps [lycheeverse/lychee-action](https://github.com/lycheeverse/lychee-action) from 1.10.0 to 2.0.2.
- [Release notes](https://github.com/lycheeverse/lychee-action/releases)
- [Commits](lycheeverse/lychee-action@v1.10.0...v2.0.2)

---
updated-dependencies:
- dependency-name: lycheeverse/lychee-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

* Don't fail lychee-action on broken links

Xref lycheeverse/lychee-action#85

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Wei Ji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants