Return the current agent state on Fixture.IsHealthy method #3982
Conversation
|
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
| return status.State == int(cproto.State_HEALTHY), nil | ||
| if status.State != int(cproto.State_HEALTHY) { | ||
| return false, fmt.Errorf("agent isn't health, current status: %s", | ||
| client.State(status.State)) |
There was a problem hiding this comment.
I understand the reason for adding this, but this kinda changes the meaning of the error in this function. The error was more that it failed to get the actual health. Now this changes it to return an error when its not healthy, that is probably not was the caller is expecting.
There was a problem hiding this comment.
This is true, but it's also why I changed it docs. Besides there is just one usage of it, that is the test which needed this change. I could change the function signature, but I believe the doc change clarifies what is returned in the error.
There was a problem hiding this comment.
Based on the way your changing it, would you be willing to change it to EnsureHealthy and remove the boolean? Doesn't seem like true/false is relevant to what this function is even doing at more, so just have it return error or nil.
There was a problem hiding this comment.
It does make sense, I just kept the name because it isn't ensuring anything, just checking the status
Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
…bug-integration-tests
|
0.0% Duplication on New Code(cherry picked from commit 58ef76d)
What does this PR do?
Modifies
Fixture.IsHealthyto return the current agent status if the agent isn't healthy.Why is it important?
When debugging a failing test it's valuable to know the current status of the agent.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works[ ] I have added an entry in./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testRelated issues
Questions to ask yourself