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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: retry only on specific exit code #57

Closed
wants to merge 3 commits into from
Closed

feat: retry only on specific exit code #57

wants to merge 3 commits into from

Conversation

andersfischernielsen
Copy link
Contributor

This will add support for retrying only on a specific error code.

We use (and love) this action in Pleo, but have certain exit codes we'd like to retry for while for other exit codes we don't.

This PR will add support for passing the retry_on_exit_code option to the action and only retry if the exit code is equal to the provided exit code to retry for (if I'm reading the logic correctly - let me know if that's not the case! 馃憤馃徏).

@andersfischernielsen
Copy link
Contributor Author

I apologize for all the auto-format changes - let me know if you'd like me to only change the else if and leave the formatting alone!

@nick-fields
Copy link
Owner

I apologize for all the auto-format changes - let me know if you'd like me to only change the else if and leave the formatting alone!

Thanks for the contribution! I like the idea, just need to clean up the commit.

Could you undo the formatting changes? Adding a test would be nice too, see other "tests" for examples of how I've been testing this. One of these days I will add actual tests...

@andersfischernielsen
Copy link
Contributor Author

Done and done 馃槃

@erwinw
Copy link

erwinw commented Apr 19, 2022

Hey @nick-fields , could this PR be merged in, please? 馃檹馃徎

@andersfischernielsen
Copy link
Contributor Author

@nick-fields I'm unable to get the test workflow to run against this PR 馃 I'm unsure if Actions are enabled for the repository since that'd prevent the workflow from running, but AFAICT it should be set up to run on all pushes (including this PR). Maybe I'm missing something obvious - I'd like to test it pre-merge 馃槃

@nick-fields
Copy link
Owner

Thanks for cleaning this up! Apologies for the delay, I was away for the holiday. I have actions disabled on contributions from forks for security reasons. I'll have to do some trickery to test it locally then I'll get this merged today.

@andersfischernielsen
Copy link
Contributor Author

No worries - and amazing! 馃檶馃徏 Let me know if you see any unexpected behaviour 馃槃

@nick-fields
Copy link
Owner

Closing in favor of #58 which is your fork pulled into a branch on this repo so that Actions/tests run before merge. See comment regarding issue with implementation.

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.

None yet

3 participants