-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat: deprecate unused snmp_trap timeout configuration option #10339
feat: deprecate unused snmp_trap timeout configuration option #10339
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.
In line 1302 in snmp_trap_test it is hitting a fatal timeout error so the package testes are failing. You can change
select {
case <-received:
case <-time.After(2 * time.Second):
t.Fatal("timed out waiting for trap to be received")
}
to just be
<-received
as we will no longer be timing out
Whoops, I was indeed a bit too quick with find and replace. It still makes sense to keep it in the tests.. |
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.
Looks great to me. Thanks for using the deprecation framework @Hipska! :-)
Hmm I am ready to merge but it says "This branch has conflicts that must be resolved"? |
Well yeah You merged #10373 in between, where comments have been added where I removed it from example config… |
Do you plan to update the MP then? |
I kept the comments on the Readme file, but removed it from generated config. This seems most sane situation to me, as generated config is used for new users of telegraf/plugin and it doesn't make much sense to introduce them to an already unused config option. |
👍 This pull request doesn't change the Telegraf binary size 📦 Looks like new artifacts were built from this PR. Expand this list to get them here ! 🐯Artifact URLs |
…influxdata#10339)" This reverts commit 7f20e71.
Required for all PRs:
Since
snmptranslate
isn't used anymore, add deprecation notice to its timeout configuration option, and remove it from example config.