Fix lustre2 input plugin config parse regression #6114
Conversation
GeorgeMac
added
fix
GeorgeMac
changed the title
|
@frroberts will reverting this name change cause issues with your previous change #5771 ? |
|
I don't see why it would cause any issues. (Unfortunately I do not have access to a real system to test on right now.) |
| @@ -330,3 +333,38 @@ func TestLustre2GeneratesJobstatsMetrics(t *testing.T) { | |||
| err = os.RemoveAll(os.TempDir() + "/telegraf") | |||
| require.NoError(t, err) | |||
| } | |||
|
|
|||
| func TestLustre2CanParseConfiguration(t *testing.T) { | |||
There was a problem hiding this comment.
Just an observation. I have mixed feelings on this test, you could imagine having tests like this for all plugins and overall I think if we did this it would be too burdensome in it's current form. On the other hand obviously it does check for a valid bug.
There was a problem hiding this comment.
I agree that in this form it would be burdensome. I actually had mentioned about having a test like this everywhere with @goller.
Would a kind of test harness for all plugins be worthwhile?
e.g.
import "github.com/influxdata/telegraf/internal/harness"
func TestHarness(t *testing.T) {
harness.TestInputPlugin(t, "some_plugin", &SomePlugin{})
}Which would validate the plugin has been registered under some_plugin. Parse the result of plugin.SamplePlugin() and assert it against the provided telegraf.Input.
Could have one per plugin type and capture some high-level criteria like this.
There was a problem hiding this comment.
I guess I'm ambivalent, it's not something that has historically been a huge source of bugs, so it might just not be worth the effort or a situation where we have bigger fish to fry. We could definitely provide a better interface to the config package to ease toml string -> plugin, but I'm not entirely sure how the harness would work. It seems like you need to provide at a minimum the input TOML with all options set, and a filled plugin struct to compare against.
There was a problem hiding this comment.
Yeah my illustration is lacking a little. For this PR shall I just leave it as is or remove the test?
The harness would ideally test all configurable fields, so the sample would need uncommenting in part if it were used. This could be done with a simple regexp, but that may be brittle and difficult to explain.
(cherry picked from commit 56c6539)
This closes #6107
PR contains a test definition to capture the regression scenario and an associated fix.
Oustanding question:
Was there a desire to parse these new
ost_jobstatandmds_jobstatfields also? Do we need to support those field names as aliases?Required for all PRs:
Associated README.md updated.