-
Notifications
You must be signed in to change notification settings - Fork 875
fix(cli/ssh): retry on autostart conflict #16058
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
Conversation
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.
👍
It looks like macOS runner is failing due to race as I feared. I can't think of a good way to enforce that this fix is hit without internal testing. Essentially I'd want to either mock the client or the API to block requests to We could also just remove the count assertion but then we have no way to know that we're actually hitting the edge case. |
8e9690a
to
e5bee0b
Compare
Thanks for the suggestions. Quartz is a bit too distant / side-effecty to properly do this I think 😅. I'll look into dbmock but that only works if it actually passes the invocation to a real DB. Don't want to mock all DB calls required to do this E2E so might be easiest via some kind of middleware used in tests where you can intercept any request before passing it on to the next handler. |
That's actually pretty nice! It seems like it should be simple enough to allow wrapping |
ced390c
to
042bd42
Compare
// Intercept builds to synchronize execution of the SSH command. | ||
// The purpose here is to make sure all commands try to trigger | ||
// a start build of the workspace. | ||
isFirstBuild := true |
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.
nit: maybe make this an atomic.Bool
or a sync.Once
?
Fixes #13032