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 Webhook Action type on client #29818
Conversation
Pinging @elastic/es-ui |
💚 Build Succeeded |
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.
Tested locally, code LGTM
💔 Build Failed |
retest |
💔 Build Failed |
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.
Tested locally, fixed the bug reported by the user. Code LGTM! Just found a couple typos in comments. Great work acting on this so quickly, Seb!
* is where it seems that this is read. But I can't access that component navigatint the UI | ||
* | ||
*/ | ||
// static typeName = i18n.translate('xpack.watcher.models.webhookAction.typeName', { |
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.
Is this commented-out code needed?
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 was hoping the reviewer(s) to be able to answer this commented code. 😊 Because it has been copied from other watcher action but those static variables don't seem to be used anywhere in the UI.
@bmcconaghy do you know if there used to be a UI form to define the watcher actions? (instead of the advanced JSON editor that we have now). It seems that those static properties were there for that purpose.
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 think there was a draft of such a feature but AFAICR it never made it into an actual release. @chrisronline may remember differently/better.
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 am not familiar with those comments at all unfortunately
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.
Ok thanks @chrisronline
* the "advanced watcher" creation through the JSON editor. | ||
* | ||
* ---> ./components/watch_actions/components/watch_action/watch_actions.html | ||
* is where it seems that this is read. But I can't access that component navigatint the UI |
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.
Typo: navigatint -> in
/** | ||
* NOTE: | ||
* | ||
* I don't seem to find in the UI where those static properties are actuall used. |
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.
Typo: actuall -> actually
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.
Don' look at the typo, look at the text! 😄 hahaha. This will be eventually removed.
Retest |
💔 Build Failed |
Looks like these CI failures are the same ones intermittently occurring on |
CI has been fixed in |
💚 Build Succeeded |
I am going to merge this PR so we have a fix for the #29787 bug. I will create a separate PR for the other missing Watcher actions. |
* Add missing Webhook action on client
* Add missing Webhook action on client
Follow-up PR with other actions is here: #30043 |
This is a step towards addressing #18059. |
💔 Build Failed |
@sebelga @cjcenizal when I try to test this PR - I get error. I am using the watch in description. This is on BC1 of 6.6.1. Can you please tell me whats happening? Logs:
|
@bhavyarm I believe this only made it into 6.6.1. |
@cjcenizal apologies 6.6.1. Sorry too many releases. The error I am seeing is on 6.6.1. I have corrected my comment. |
This works fine apart from a copy bug. Logging it in a bit. Thanks @sebelga |
This PR adds the missing action types on the Watcher UI client. Currently, we have an issue related to the Webhook action missing but there are other action types missing.
To unblock #29787 this PR could be merged for the Webhook action that was missing. I will add though the other missing action types to avoid future bugs.
Note: There are several static properties defined on each action class but I can't seem to find where in the UI they are actually used. I put a comment at the bottom of the
webhook.action.js
file about it.How to test
What we want to test is making sure that the
body
of the HTTP request contains the corresonding values that we have defined in the JSON editor (and of course that the application does not crash) when saving the watcher.