-
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(inputs.mikrotik): Add plugin #16081
base: master
Are you sure you want to change the base?
Conversation
Thanks so much for the pull request! |
e557986
to
b9c5115
Compare
This three issues I am having:
|
#16087 I believe should fix the first two, and the 3rd is an unrelated flaky test, so nothing needed here (other than a rebase/merge after the linked PR merges) |
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.
Thanks @s-r-engineer for your contribution! I want to encourage you to simplify your code! Why not defining known tag
and field
names of responses and decode the JSON responses to map[string]string
? Then you can iterate over the map and decode the value accordingly? This would get rid of the reflection code and in general would simplify the code...
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
@s-r-engineer my review comments are that, comments about the code. They are not meant to criticize or insult you as a programmer or anyone else. If you understood it this way, please accept my apologies. |
@srebhan I never was insulted whatsoever. I am sorry if I made you feel like that. |
@srebhan what to do with this? I am not sure why this is an issue. https://app.circleci.com/jobs/github/influxdata/telegraf/363717 |
… tests (it is for sure but I still hope not) (influxdata#16080)
…the issue is in secret storage (influxdata#16080)
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
Summary
The idea is to be able to connect and gather metrics from the Mikrotik's RouterOS
Checklist
Related issues
resolves #16080