chore: Fix linter findings for Windows (part4) #13246
Conversation
Hipska
added
the
ready for final review
There was a problem hiding this comment.
Thanks for the fixes @zak-pawel. Please revert the massive change to ethtool and leave the file-structure as-is for now. Similar for diskio where duplicating the general struct will result in pain (and forgotten sync-ups) when adding config-options.
plugins/inputs/diskio/diskio.go
Outdated
| type DiskIO struct { | ||
| ps system.PS | ||
|
|
||
| Devices []string | ||
| DeviceTags []string | ||
| NameTemplates []string | ||
| SkipSerialNumber bool | ||
|
|
||
| Log telegraf.Logger | ||
|
|
||
| infoCache map[string]diskInfoCache | ||
| deviceFilter filter.Filter | ||
| } |
There was a problem hiding this comment.
The only thing I don't like about duplicating this struct is that you now have two places to edit if you add an option. So I would prefer not to do this. We could move the internal fields to a struct that is defined by linux/not-linux though...
There was a problem hiding this comment.
I think that we need to have dedicated talk about best solution. For now, I'm reverting changes for diskio plugin.
plugins/inputs/ethtool/ethtool.go
Outdated
| const ( | ||
| pluginName = "ethtool" | ||
| tagInterface = "interface" | ||
| tagNamespace = "namespace" | ||
| tagDriverName = "driver" | ||
| fieldInterfaceUp = "interface_up" | ||
| ) |
There was a problem hiding this comment.
Instead of completely changing the setup you should just move those definitions into the _linux.go file!
There was a problem hiding this comment.
Instead of completely changing the setup you should just move those definitions into the
_linux.gofile!
@srebhan Can you elaborate how would this help with following findings for non-linux OSes?
plugins/inputs/ethtool/ethtool.go:43:2 unused field `interfaceFilter` is unused
plugins/inputs/ethtool/ethtool.go:44:2 unused field `namespaceFilter` is unused
plugins/inputs/ethtool/ethtool.go:45:2 unused field `includeNamespaces` is unused
plugins/inputs/ethtool/ethtool.go:48:2 unused field `command` is unused
I strongly believe that situation in ethtool plugin is much clearer right now. Content of ethtool.go and ethtool_linux.go were merged into single ethtool.go for linux only. Other OSes are gently handled by ethtool_notlinux.go.
There was a problem hiding this comment.
The reason why I don't like the structural changes is
- It is a massive change not necessarily required to accommodate the linter issue.
- The structure is now again different from other plugins that are platform dependent!
Can you elaborate how would this help with following findings for non-linux OSes?
Those constants are used in the linux-only build and are undefined in the non-linux build. Doesn't that solve the linter issues?
There was a problem hiding this comment.
I must admit that I don't know why you are talking about those constants. Problem is only with struct fields:
plugins/inputs/ethtool/ethtool.go:43:2 unused field `interfaceFilter` is unused
plugins/inputs/ethtool/ethtool.go:44:2 unused field `namespaceFilter` is unused
plugins/inputs/ethtool/ethtool.go:45:2 unused field `includeNamespaces` is unused
plugins/inputs/ethtool/ethtool.go:48:2 unused field `command` is unused
I think that we need to have dedicated talk about best solution. For now, I'm reverting changes for ethtool plugin.
powersj
removed
the
ready for final review
|
After merging this PR, these will be the only findings for both |
telegraf-tiger
bot
removed
the
waiting for response
powersj
added
the
ready for final review
@powersj This code is based on current upstream master branch Don't know why still failing... |
oh I did not read this close enough, sure enough it is :( @srebhan updated that to change from |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
It is last part of the bigger job: #13036
After merging this PR, linter CI job for Windows and Darwin can be enabled.
Fixes for following findings have been made:
Few notes:
cmd/telegraf/telegraf_windows.go- seems thatrun()has been unused since https://github.com/influxdata/telegraf/pull/11700/files#diff-e92b823b01e2753ed78851f62eeed84d917d61d172b3666312c1ae5289ab0ff6L40-L51diskioplugin supportswindows(I've tested that) - few movements were performed to avoidunusedfindings forwindowsanddarwin.ethtool- few movements were performed to avoidunusedfindings forwindowsanddarwin.filecount- few movements were performed to avoidunusedfindings forwindows. They can be rollbacked after this FIlestat plugin and super asterisks not working on windows? #6248 is fixed.ping- to get rid offunction-result-limit, similar operation was made as for https://github.com/influxdata/telegraf/pull/10066/files#diff-ee4d8ab6f17ad76a4f4297c7e1a6b8b2e3fdf1935ecbee0bc98b00b075d9a46f