Skip to content
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

Merged
merged 3 commits into from
Jan 10, 2022
Merged

feat: deprecate unused snmp_trap timeout configuration option #10339

merged 3 commits into from
Jan 10, 2022

Conversation

Hipska
Copy link
Contributor

@Hipska Hipska commented Dec 23, 2021

Required for all PRs:

Since snmptranslate isn't used anymore, add deprecation notice to its timeout configuration option, and remove it from example config.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Dec 23, 2021
@Hipska Hipska added area/snmp plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Dec 23, 2021
Copy link
Contributor

@MyaLongmire MyaLongmire left a 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

@Hipska
Copy link
Contributor Author

Hipska commented Dec 23, 2021

Whoops, I was indeed a bit too quick with find and replace. It still makes sense to keep it in the tests..

Copy link
Member

@srebhan srebhan left a 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! :-)

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jan 6, 2022
@powersj
Copy link
Contributor

powersj commented Jan 6, 2022

Hmm I am ready to merge but it says "This branch has conflicts that must be resolved"?

@Hipska
Copy link
Contributor Author

Hipska commented Jan 6, 2022

Well yeah You merged #10373 in between, where comments have been added where I removed it from example config…

@powersj
Copy link
Contributor

powersj commented Jan 7, 2022

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?

@Hipska Hipska requested a review from powersj January 10, 2022 09:21
@Hipska
Copy link
Contributor Author

Hipska commented Jan 10, 2022

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.

@powersj powersj merged commit 7f20e71 into influxdata:master Jan 10, 2022
@Hipska Hipska deleted the snmp/translate_duration branch January 10, 2022 14:47
nward added a commit to nward/telegraf that referenced this pull request Jan 12, 2022
powersj pushed a commit to powersj/telegraf that referenced this pull request Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/snmp feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants