-
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
fix issue with shim use of config.Duration #7996
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# testing custom field types | ||
[[inputs.test]] | ||
duration = "3s" | ||
size = "3MB" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,3 +54,24 @@ func TestLoadingConfig(t *testing.T) { | |
|
||
require.Len(t, c.Processors, 1) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test belongs in config/config_test.go instead of here. TestConfig_LoadSpecialTypes is similar. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can't have config require reverse_dns due to cyclical dependencies. Since it mentions both, it has to go here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .. though, a config_test package might work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this with fresh eyes, the test above it is almost identical in setup. I'm not so sure that it belongs in config. The functionality in config is covered by another test (in config_test above), and this was just an extra sanity test I added to make sure the plugin had what it needed, and wasn't falling back on default values. In the end this was not needed (because it's not a regression test per se, and didn't highlight the bug I was fixing), but I figured it didn't hurt to add the test to the test suite. I'd recommend leaving it as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand it's convenient to put it here because of dependencies, but it doesn't belong here because it's testing config parsing, not reverse dns functionality. I'll leave it to you to move if you want. |
||
func TestConfigDuration(t *testing.T) { | ||
c := config.NewConfig() | ||
err := c.LoadConfigData([]byte(` | ||
[[processors.reverse_dns]] | ||
cache_ttl = "3h" | ||
lookup_timeout = "17s" | ||
max_parallel_lookups = 13 | ||
ordered = true | ||
[[processors.reverse_dns.lookup]] | ||
field = "source_ip" | ||
dest = "source_name" | ||
`)) | ||
require.NoError(t, err) | ||
require.Len(t, c.Processors, 1) | ||
p := c.Processors[0].Processor.(*ReverseDNS) | ||
require.EqualValues(t, p.CacheTTL, 3*time.Hour) | ||
require.EqualValues(t, p.LookupTimeout, 17*time.Second) | ||
require.Equal(t, p.MaxParallelLookups, 13) | ||
require.Equal(t, p.Ordered, true) | ||
} |
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.
This is a good change but looks unrelated. Was it supposed to be in this pr?
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.
Unrelated; just don't want to make a new pr for one line. :D