Warn on & skip workflow runs for certain "broken" GitHub workflows #151
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jwodder
changed the title
|
looks good. updated in tinuous-dev conda env of datalad@smaug which is used by cron jobs and testing on datalad-extensions. Since needs to wait for all those retries -- I will just check it later. |
|
ha -- it crashed with 502 not 500 I have checked prior emails -- they were about 500s. I guess we should adjust condition so it could be 500 or 502. I will try with that. |
yarikoptic
reviewed
Oct 11, 2022
src/tinuous/github.py
Outdated
Comment on lines
112
to
113
| and reason.args[0] | ||
| == ResponseError.SPECIFIC_ERROR.format(status_code=500) |
There was a problem hiding this comment.
I will try now
Suggested change
| and reason.args[0] | |
| == ResponseError.SPECIFIC_ERROR.format(status_code=500) | |
| and (reason.args[0] in | |
| [ResponseError.SPECIFIC_ERROR.format(status_code=500), | |
| ResponseError.SPECIFIC_ERROR.format(status_code=502)]) |
There was a problem hiding this comment.
confirming that smth like that (I reentered manually) worked but needs adjustment of the log message below to log actual status_code - not hard-coded 500 -- please adjust accordingly and let's get it merged/released (adding a label).
There was a problem hiding this comment.
@yarikoptic Should the code just catch 500 and 502 here, or also 503 and 504 (the same as the status codes that are already retried)?
There was a problem hiding this comment.
Totally up to you - feel free to add those 5xx into the mix here as well .
- Originally I thought about it and because we are dealing with specific workaround, thought we better keep to 500 and 502 until we run into the case where any other 5xx shows up for this particular scenario since we do not want to skip any other similar case I guess.
- Since we do test for the
sinceand presence of workflow file I think it would be ok if we skip on any 5xx.
Codecov Report
@@ Coverage Diff @@
## master #151 +/- ##
==========================================
+ Coverage 69.15% 69.88% +0.72%
==========================================
Files 10 10
Lines 976 993 +17
Branches 183 152 -31
==========================================
+ Hits 675 694 +19
+ Misses 244 242 -2
Partials 57 57
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
yarikoptic
reviewed
Oct 11, 2022
| @@ -34,6 +35,12 @@ | |||
| sanitize_pathname, | |||
| ) | |||
|
|
|||
| RETRY_STATUSES = [500, 502, 503, 504] | |||
There was a problem hiding this comment.
eh, I was trying to avoid "equating" the RETRY_STATUSES to all the statues we might want to skip on (only 5xxs). ATM they are indeed identically but in general not necessarily the same, so IMHO it mixes the semantics abit ... but since functionally is ok ATM, let's proceed. Thank you @jwodder ! Merging
jwodder
added a commit
that referenced
this pull request
Nov 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #150.