-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 Index, HipChat, PagerDuty, and Jira Action types on the client #30043
[Watcher] Add Index, HipChat, PagerDuty, and Jira Action types on the client #30043
Conversation
Pinging @elastic/es-ui |
💔 Build Failed |
💚 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.
Haven't had time to test locally yet, but the code LGTM! Had some suggestions which we can address separately.
super(props); | ||
|
||
allFields.forEach((field) => { | ||
this[field] = get(props, field); |
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 could see a potentially difficult problem to debug if we ever add support for a field which collides with one of the class properties (e.g. description
). This is compounded by the presence of properties inherited from BaseAction
. What do you think of assigning these fields to an instance property, to eliminate this possibility? For example:
this.fields = {};
allFields.forEach((field) => {
this.fields[field] = get(props, field);
});
This change would have to be applied to all of the other actions too, and if any code is depending on these dynamic properties then this change could be significant; probably too significant for this PR. Let me know if you want me to move this suggestion into a separate issue.
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.
Good point, I'll check for property collision.
}); | ||
} | ||
|
||
get upstreamJson() { |
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 didn't look very closely, but this method looks duplicated throughout all of the actions. Would it make sense to extract it into a generalized service?
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.
Yes you are right this could be optimized but as Watcher will be rewritten I just wanted to prevent future bugs in the meantime because this file was missing.
If possible, it would be very helpful to also update the description with steps for testing, e.g. various requests to make in Dev Tools, and any settings we need to change for X-Pack. |
@cjcenizal I just updated the comment to explain how to test. Can you have another look? |
💚 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.
Thanks for the steps to test! This really helped me a lot. I was able to test locally and verify the behavior works as expected. This information also helped me learn more about Watcher in general and I was able to spot a few errors in the code.
return i18n.translate('xpack.watcher.models.jiraAction.description', { | ||
defaultMessage: '{issueName} will be created in Jira', | ||
values: { | ||
issueName: this.fields && this.fields.issuetype && this.fields.issuetype.name || '' |
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.
From the Jira action docs it looks like the expected shape is:
{
"account" : "integration-account",
"fields" : {
"issue" : {
"issuetype" : {
"name": "Bug"
}
}
}
}
So I think the correct path should be this.fields.fields.issue.issuetype.name
, right?
If we're already using lodash's get
, I think we might as well use that here too:
get(this.fields, 'fields.issue.issuetype.name', '');
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.
Good catch!
defaultMessage: 'The {index} will be indexed as {docType}', | ||
values: { | ||
index: this.index, | ||
docType: this.doc_type, |
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 we need to update the fields references here:
this.index
->this.fields.index
this.doc_type
->this.fields.doc_type
return i18n.translate('xpack.watcher.models.hipchatAction.description', { | ||
defaultMessage: '{body} will be sent through Hipchat', | ||
values: { | ||
body: this.message && this.message.body || '' |
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 we need to update references of this.message
to this.fields.message
.
return i18n.translate('xpack.watcher.models.pagerDutyAction.description', { | ||
defaultMessage: '{description} will be sent to PagerDuty', | ||
values: { | ||
description: this.description, |
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.description
-> this.fields.description
allFields.forEach((field) => { | ||
this[field] = get(props, field); | ||
this.fields[field] = get(props, field); | ||
}); | ||
|
||
this.fullPath = this.url ? this.url : this.host + this.port + this.path; |
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.
const { url, host, port, path } = this.fields;
this.fullPath = url ? url : host + port + path;
@cjcenizal updated the PR with your changes. Can you have another look? cheers! |
💔 Build Failed |
💚 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.
Looks great, Seb!
This is a step towards addressing #18059. |
Since it has been backported to 6.6 and 6.7 branches can we have version labels added to the issue? Which 6.6 version is going to include the fix? |
Thanks for the reminder @immon. This will ship in 6.6.2 and 6.7.0. |
This PR adds the missing action types on the Watcher UI client. There recently was an issue (#29787) related to the Webhook action missing and it came to the light that there were other action types missing.
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.Hiphat action
You should get a toast error message, which is ok. The important part is that the body of the request that has been sent contains the action that we have just defined. It should be under
watch.actions.my_actions
of the PUT requestbody
.Repeat the same test with the other action types:
PagerDuty action
Jira action
Index action