-
Notifications
You must be signed in to change notification settings - Fork 162
Determine workflow run id more robustly to avoid race conditions #37
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
Determine workflow run id more robustly to avoid race conditions #37
Conversation
…rval with api and docs
…ate 'INPUT_WAIT_INTERVAL' in the ##Testing section of the README.
…gh a configurable last_workflow_interval input variable (defaults to 0). Redefine 'inputs' variable to be 'client_payload'. Lastly, using #bash, which uses /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
|
If two runs are started very close together, this logic will see two new runs. It's impossible to tell which is the one we triggered, so we wait for both of them. If either of them fails (and we're propagating failures) we fail this one. |
The Alpine base image uses the busybox implementation of sh, so we need to use the same shell when testing entrypoint.sh outside of Docker.
|
@keithconvictional Any chance of some movement on this? The behaviour of the current version is pretty flaky and this fixes it. |
This prevents runners from using a stale base and is better practice anyway
keithconvictional
left a comment
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.
LGTM
|
Changes merged here reverted #41 😭 |
|
@duhow That's unfortunate. Looks like some of those changes got missed while resolving merge conflicts. The most important one is the Do you plan to submit a new PR? |
|
@duhow @neilmayhew - Sorry about that. If you put a new PR, I'll review by end of week. The slow down was testing, and I have a better way at testing these changes now. |
|
@keithconvictional I've pushed a new PR (#47) |
This builds on #34