-
Notifications
You must be signed in to change notification settings - Fork 28
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
more multi thread coordination need to avoid panics in TestStartBuildRunFollowLog #41
more multi thread coordination need to avoid panics in TestStartBuildRunFollowLog #41
Conversation
hmmm I have more work to do
|
cf1656a
to
991b7df
Compare
/approve I've pinged @adambkaplan in shipwright slack about adjusting the permissions in this repo so I can re-run the unit tests a few more times before we potentially merge this one |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabemontero The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
thanks @adambkaplan ... rerunning test jobs now ... will do that a few times today, see how clean they are |
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.
I've tested it locally as well, it looks solid!
/lgtm |
Changes
github actions back e2e runs turned up this panic in the new unit test I added in one of its recent runs:
need to employ my watch lock a bit more in the test along with the use of Gocsched
but I still avoid a blanket, sleep always approach to guarantee Run() init has occurred
/assign @otaviof
/assign @coreydaley
/kind bug
Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes