-
Notifications
You must be signed in to change notification settings - Fork 813
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
refactor: Implemented using the standard library #2847
Conversation
Build Failed 😱 Build Id: 7b362c71-50b5-42b2-822a-f921ec3566aa To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
/retest |
@aimuz Sadly retest doesn't work in this repo but I kicked off a new test. This LGTM. We're in feature freeze and this is more of a cleanup, so I'm holding off until thaw to merge. |
Build Failed 😱 Build Id: c9194ec2-bf68-44f5-b086-92ef74ad7bad To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Bleh. Both of these failed on:
which is #2018. |
Build Failed 😱 Build Id: 37bf0885-3d99-44d4-88b6-238a0b098e61 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 2db3d081-66cf-43d1-931c-65faeec37241 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
@aimuz You'll need to force-push this branch again to get the commit hash to change to get past that flake. It looks like you need to rebase anyways, so you could do that, and it'll retest after you push the rebase. If it again fails in the same place, you can re-push after |
We can also wait until after stable, and update the branch when something else is merged. Should be fine. |
Thanks for your replies @zmerlynn @markmandel , I'll try the @zmerlynn method first |
861fd1d
to
dd07d5c
Compare
Build Failed 😱 Build Id: 6dd688ed-681b-493e-88a3-45b92e77fa2d To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 02c5c854-771a-4b6f-960b-3ca83f0fd4cb To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: af3b2c68-6e03-43ab-9844-b2e6f95e3a4c The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 338c8e61-7bac-41e7-903a-7155825465cb The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Signed-off-by: aimuz <mr.imuz@gmail.com>
"os/signal" | ||
"syscall" | ||
) | ||
|
||
// NewSigKillContext returns a Context that cancels when os.Interrupt or os.Kill is received | ||
func NewSigKillContext() context.Context { | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
c := make(chan os.Signal, 2) |
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.
@markmandel - I noticed that the library function creates a channel with size 1 regardless of how many signals are being tracked: https://cs.opensource.google/go/go/+/refs/tags/go1.19.4:src/os/signal/signal.go;l=284. I assume that since it's part of the std library and the API is designed to handle multiple signals that it is safe (or even correct) to set the channel to size 1 but wanted to know if there was a reason that 5 years ago you set this to size 2: 726af15#diff-e7b27b1d977569544a1feedd454f855f61c641a2f35bf9fe75d4bc15c7cefb35
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.
Honestly - not that I can remember! $10 says I copied from some other example that copied from another example?
I'd give more confidence to the Go std library than code I wrote 5 years ago 😄
Build Succeeded 👏 Build Id: d4255653-3760-4470-8bc1-b7f38605022a The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aimuz, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: aimuz <mr.imuz@gmail.com>
Signed-off-by: aimuz mr.imuz@gmail.com
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer:
This pr may not be of much value, but I think it is a better practice to use the standard library implementation