-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Watcher] Add support for ERROR and UNKNOWN watch action statuses #55092
[Watcher] Add support for ERROR and UNKNOWN watch action statuses #55092
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
} | ||
) | ||
); | ||
return ACTION_STATES.UNKNOWN; |
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 seemed more appropriate to mark the action in an unknown state, rather than throwing an error and making the watch list view unusable.
The only potential outstanding question is what the overall watch status should return in this scenario. I did not make any code changes to how this is handled, so it currently will return OK
. Thoughts?
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 definitely agree with rather keeping the view usable in case of a bad status (i.e., catering for this condition here definitely seems like the correct thing to do). Overall I am happy with this approach 👍
The one advantage of throwing in this case, however, is that we were able to discover this bug which I think may have remained hidden otherwise. So being totally silent also has a cost (as you've noted).
Judging how action_status and watch_status hang together, it would be cool to be able to say that if the last action status was unknown, then the watch_status is unknown? Remaining in this state should be a red flag to the user + they can still use the UI, we should communicate it's a possible bug. This could be a separate unit of work to this fix though!
Let me know what you think? We could revert this line for now and address later, or commit and address later. I'm happy with either for now.
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.
@jloleysens I like this idea a lot - thanks! I opened #55410 to address separately.
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.
Left a comment, would like to hear your thoughts there, but we don't have to block for that on the this.lastAcknowledged < this.lastExecution
fix.
x-pack/legacy/plugins/watcher/server/np_ready/models/action_status/action_status.js
Outdated
Show resolved
Hide resolved
} | ||
) | ||
); | ||
return ACTION_STATES.UNKNOWN; |
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 definitely agree with rather keeping the view usable in case of a bad status (i.e., catering for this condition here definitely seems like the correct thing to do). Overall I am happy with this approach 👍
The one advantage of throwing in this case, however, is that we were able to discover this bug which I think may have remained hidden otherwise. So being totally silent also has a cost (as you've noted).
Judging how action_status and watch_status hang together, it would be cool to be able to say that if the last action status was unknown, then the watch_status is unknown? Remaining in this state should be a red flag to the user + they can still use the UI, we should communicate it's a possible bug. This could be a separate unit of work to this fix though!
Let me know what you think? We could revert this line for now and address later, or commit and address later. I'm happy with either for now.
…atus/action_status.js Co-Authored-By: Jean-Louis Leysens <jloleysens@gmail.com>
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…t-of-legacy * 'master' of github.com:elastic/kibana: [Watcher] Add support for additional watch action statuses (elastic#55092) [ML] Fix entity controls update. (elastic#55524) [ML] Fixing ML's watcher integration (elastic#55439) [ML] Fixes permissions checks for data visualizer create job links (elastic#55431) [SearchProfiler] Move out of legacy (elastic#55331) # Conflicts: # x-pack/plugins/watcher/server/models/action_status/action_status.js
Fixes #54730
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Documentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityRelease note
This change fixes a bug in which the UI did not load the watch overview page if one of user's watches had a watch action in an invalid state.