Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[process-agent] Remove check intervals from pkg/process/config #14878
[process-agent] Remove check intervals from pkg/process/config #14878
Changes from 1 commit
00f86c4
acbffa9
f4229d9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The actual comparison is
< discoveryMinInterval
, not<= discoveryMinInterval
, so this log line should reflect that.Also, we're using the minimum, not the default, so I fixed that in the suggestion as well.
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.
Ah, nice catch, let's fix this.
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.
It might not be a good idea to log an error here. AFAIK most customers don't override the default check intervals, and I don't necessarily think we should be telling them that they need to do so.
Nit: Also, I think the convention is to not explicitly ignore the error for logs, do you mind removing the
_ =
. In the past, people have been confused when I added this and it would be nice to keep a consistent style.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.
Point taken, this was moved rather mechanically though, so the logging part exists today.
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.
Oh interesting. I didn't notice that. I think it might be worthwhile removing while we're here, but I'll give you the final decision in this
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.
I agree with you, it was probably done this way when we weren't binding defaults to these in
pkg/config
so removing the log makes sense!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.
Do you mind adding test cases for the
ProcessEventsCheck
, and the default case? Should be an easy way to boost our code coverage a bit 😄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.
It might fit better in a separate function rather than a separate test case, but I'll leave that decision up to you
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.
Yes, I will add tests for these two other branches.
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.
Added tests and found bugs, best suggestion ever 😄