-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 prometheusreceiver TA/scrape_config validation logic #30135
Conversation
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
9085c72
to
e2c168e
Compare
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
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.
Though, I don't agree with the overall approach of forcing this PR before the refactoring PR that showed this problem just because the author blocked the other PR, I am ok approving since this fixes a problem.
if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil { | ||
return errors.New("no Prometheus scrape_configs or target_allocator set") | ||
} | ||
|
||
if promConfig != nil { |
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 would suggest moving this to the validatePromConfig
simplifies indentation.
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.
That's where it was and you told me to move it.
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 told to move the following code in the config.Validate
if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil {
return errors.New("no Prometheus scrape_configs or target_allocator set")
}
And in this comment I asked to move in the validatePromConfig
if promConfig == nil {
return nil
}
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.
Author ignored reviewer comments, please at least disagree with them, not just mark them as resolve like #30135 (comment)
if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil { | ||
return errors.New("no Prometheus scrape_configs or target_allocator set") | ||
} | ||
|
||
if promConfig != nil { |
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 told to move the following code in the config.Validate
if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil {
return errors.New("no Prometheus scrape_configs or target_allocator set")
}
And in this comment I asked to move in the validatePromConfig
if promConfig == nil {
return nil
}
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
This PR is a fork from #30135 where the author did not respond to the comments. I applied all suggestions because as a maintainer is my responsibility to ensure we fix bugs and merge PRs. Thanks @Aneurysm9 for the first commit. --------- Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com> Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Co-authored-by: Anthony J Mirabella <a9@aneurysm9.com>
PR forked and comments fixed in #30181. |
@bogdandrutu I was on vacation when you took it upon yourself to fork this to make irrelevant changes. You approved the exact same code you later blocked. I took that to mean that the comments you made were not required to be addressed and they did not appear to me to be relevant or useful in light of the in-flight refactoring PR you have open. In the future if you expect that comments will lead to changes perhaps don't also indicate your approval. |
I would kindly ask you to avoid calling changes asked by peers/reviewers as "irrelevant".
I did approve the change with some comments, for me that means that I trust you will resolve the comments, but they are not significant to require a new review.
Thank you for your feedback, I will not approve in the future any PR until all comments are reviewed (with other approvers/maintainers I assume that they will understand myself as I want this changes, but I trust them to do the changes and because they are simple enough I don't need a re-review). |
…try#30181) This PR is a fork from open-telemetry#30135 where the author did not respond to the comments. I applied all suggestions because as a maintainer is my responsibility to ensure we fix bugs and merge PRs. Thanks @Aneurysm9 for the first commit. --------- Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com> Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Co-authored-by: Anthony J Mirabella <a9@aneurysm9.com>
Description: Fixes faulty logic regarding requirement that either Prometheus
scrape_configs
or target allocator configuration be provided.Testing: Tests are added that ensure valid combinations are accepted.