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

feat: retry only on specific exit code #58

Merged
merged 7 commits into from Apr 26, 2022

Conversation

nick-fields
Copy link
Owner

I just pulled the changes from the forked PR #57 into a branch so that Actions and tests would run.

@nick-fields nick-fields force-pushed the nrf/retry-on-specific-exit-code branch from 809a10b to 679f4bb Compare April 19, 2022 12:32
expected: 3
actual: ${{ steps.retry_on_exit_code_expected.outputs.total_attempts }}

- name: retry_on_exit_code (with unexpected error code)
Copy link
Owner Author

Choose a reason for hiding this comment

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

@andersfischernielsen @erwinw

Can you take a look at this test failure? I made a few tweaks to your tests, first being that a duplicate id was used which caused the entire workflow to fail, then I added assertions to your two tests.

The assertion is failing for your retry_on_exit_code (with unexpected error code) test. I assert that if an unexpected exit code is thrown (exit code 1), and the retry_on_exit_code is set (exit code 2), then the rerun should only occur once not 3x as is happening currently. I think you need to also handle max_attempts when retry_on_exit_code is set and skip additional attempts if an unexpected exit code is returned.

Copy link

Choose a reason for hiding this comment

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

Hey @nick-fields , well danged, I've poked gently asked @andersfischernielsen to have a look at this!

@andersfischernielsen
Copy link
Contributor

andersfischernielsen commented Apr 25, 2022

@nick-fields Sorry about the duped IDs 😬

I’m pretty sure (take this with a grain of salt due to tired brain) that moving the check for the specific exit code to a separate else if would take care of the issue we’re seeing. AFAICT the existing return code checks interfere with just giving up and not retrying on our provided retry exit code 🤔

In other words, what I'd change the implementation to is:

image

As far as I understand the pre-existing code, this would allow us to blindly throw if a specific exit code is given _and_ we don't see that given exit code after the first failure, in turn leading to no retries. LMK if I'm misunderstanding the behaviour here 🙏🏼

Great that moving the PR allows Actions to run, but I'm unable to contribute to this PR due to missing access rights. How would you like me to contribute a fix to the failing test?

@nick-fields
Copy link
Owner Author

@nick-fields Sorry about the duped IDs 😬

I’m pretty sure (take this with a grain of salt due to tired brain) that moving the check for the specific exit code to a separate else if would take care of the issue we’re seeing. AFAICT the existing return code checks interfere with just giving up and not retrying on our provided retry exit code 🤔

In other words, what I'd change the implementation to is:

image As far as I understand the pre-existing code, this would allow us to blindly throw if a specific exit code is given _and_ we don't see that given exit code after the first failure, in turn leading to no retries. LMK if I'm misunderstanding the behaviour here 🙏🏼

Great that moving the PR allows Actions to run, but I'm unable to contribute to this PR due to missing access rights. How would you like me to contribute a fix to the failing test?

It was easier to just pull your proposed fix in and test it. Looks like it worked! Merging now, and thanks for the contribution.

@nick-fields nick-fields merged commit f227091 into master Apr 26, 2022
@github-actions
Copy link

🎉 This PR is included in version 2.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@nick-fields nick-fields deleted the nrf/retry-on-specific-exit-code branch August 4, 2022 14:20
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.

None yet

3 participants