-
-
Notifications
You must be signed in to change notification settings - Fork 12
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: New input parameter for the action and linear backoff strategy for retries #267
Conversation
4cd280d
to
3d2c333
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
==========================================
+ Coverage 96.75% 96.81% +0.05%
==========================================
Files 6 6
Lines 555 565 +10
Branches 111 112 +1
==========================================
+ Hits 537 547 +10
Misses 17 17
Partials 1 1 ☔ View full report in Codecov by Sentry. |
@Codex- Tests The tests are passing now, and I noticed (if I'm not mistaken) that the token access issue for the action is expected when triggered from a PR. What do you think about the PR? This new functionality would be very helpful for us. Thanks! |
@Yermanaco will give a proper review when I can, thanks for the PR Christmas and New Years period is always a bit chaotic 😅 |
Happy new Year @Codex- ! Just a kindly reminder 😄 |
…ear backoff strategy for retries
17af0e0
to
1674d6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good overall, just a couple of small things then I think we can get this released today
@@ -178,7 +181,14 @@ export async function getRunIdAndUrl({ | |||
core.info(`No Run IDs found for workflow, attempt ${attemptNo}...`); | |||
} | |||
|
|||
await sleep(constants.WORKFLOW_JOB_STEPS_RETRY_MS); | |||
const waitTime = Math.min( | |||
workflowJobStepsRetryMs * attemptNo, // Lineal backoff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lineal backoff makes sense to me, but does change the behaviour people expect when setting workflow_job_steps_retry_seconds
. I'm generally inclined to try follow the principle of least surprise.
If you, as a user, specify workflow_job_steps_retry_seconds
to be 10 seconds and then find your workflow is taking 60 seconds for 3 attempts, would this surprise you?
You may expect this to behave such that:
- Attempt 1: 10s
- Attempt 2: 10s
- Attempt 3: 10s
But implicitly using lineal backoff with the attempt count as the magnitude
- Attempt 1: 10s
- Attempt 2: 20s
- Attempt 3: 30s
Perhaps for now just noting this behaviour in the readme will be enough:
...
workflow_job_steps_retry_seconds:
# Lineal backoff retry attempts are made where the attempt count is
# the magnitude and the scaling value is `workflow_job_steps_retry_seconds`
10 # Default: 5
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in the readme and in the action description too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you for contributing these changes ✨
I will comment on this PR when I've released them
Released, can be used with either the |
Make the WORKFLOW_JOB_STEPS_RETRY_MS variable an input parameter for the action and implement a linear backoff strategy for retries. This would be really useful for us since we run massive builds and want to flexibly space out the retry times. This way, we mitigate reaching the GitHub API rate limits.