-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add gosimple linter #9590
Add gosimple linter #9590
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.
Mostly LGTM
@@ -65,14 +65,12 @@ func (m *PluginGroup) WaitForFirstFingerprint(ctx context.Context) (<-chan struc | |||
go func() { | |||
defer wg.Done() | |||
logger.Debug("waiting on plugin manager initial fingerprint") | |||
<-manager.WaitForFirstFingerprint(ctx) |
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.
Does WaitForFirstFingerprint
have a side-effect? I'd have thought we'd want something more like:
select {
case <- manager.WaitForFirstFingerprint(ctx):
logger.Debug("finished plugin manager initial fingerprint")
case <-ctx.Done():
logger.Warn("timeout waiting for plugin manager to be ready")
}
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.
My goal here was not to change the behavior; are you saying you think it has changed, or that we should introduce this change?
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 was saying that as long as we're touching this code we should introduce this change and fix what looks like a minor bug. But that's a good point... maybe that should be under separate PR so that it's not getting lost in this linter PR.
As pointed out by [@tgross](#9590 (comment)), prior to this change we would have been blocking until all managers waited for first fingerprint rather than timing out as intended.
As pointed out by @tgross[1], prior to this change we would have been blocking until all managers waited for first fingerprint rather than timing out as intended. 1: #9590 (comment)
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
No description provided.