-
Notifications
You must be signed in to change notification settings - Fork 154
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
Return the current agent state on Fixture.IsHealthy method #3982
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
edf3901
return the current agent state on Fixture.IsHealthy method
AndersonQ cda693a
set STATE_PATH so the upgrade watcher can be launched
AndersonQ 143db53
Revert "set STATE_PATH so the upgrade watcher can be launched"
AndersonQ 6e27207
Merge branch 'main' into debug-integration-tests
AndersonQ b2e33b5
Update pkg/testing/fixture.go
AndersonQ 1cdab64
Merge branch 'main' into debug-integration-tests
AndersonQ 659995c
remove Fixture.IsHealthy bool return parameter
AndersonQ 5ff7139
Merge branch 'main' into debug-integration-tests
AndersonQ 1641e33
Merge remote-tracking branch 'origin/debug-integration-tests' into de…
AndersonQ 9eaa878
fix return
AndersonQ 08cd593
Merge branch 'main' into debug-integration-tests
AndersonQ File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does make sense, I just kept the name because it isn't ensuring anything, just checking the status