[Connectors] ServiceNow ITSM & SIR Application #105440
Merged
Conversation
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
cnasikas
added
v8.0.0
Team:ResponseOps
cnasikas
changed the title
cnasikas
force-pushed
the
sn_import_set
branch
2 times, most recently
from
37737f4 to
a7a09e5
Compare
cnasikas
force-pushed
the
sn_import_set
branch
3 times, most recently
from
42327a5 to
9fef005
Compare
cnasikas
force-pushed
the
sn_import_set
branch
from
9fef005 to
64c469f
Compare
cnasikas
force-pushed
the
sn_import_set
branch
2 times, most recently
from
439421f to
836a3b2
Compare
|
Linking to the design issue: https://github.com/elastic/stack-design-team/issues/109 |
cnasikas
force-pushed
the
sn_import_set
branch
from
24e4e52 to
fd1dbd1
Compare
cnasikas
force-pushed
the
sn_import_set
branch
2 times, most recently
from
dd69493 to
d7d7540
Compare
|
I just opened Allow connector UX to display warnings about connectors #114507 to discuss making the warning icon a framework-level capability. |
pmuellr
approved these changes
Oct 11, 2021
There was a problem hiding this comment.
I'm marked this as approved - per separate conversation, the issues I'd brought up in the previous review can be deferred so that we can get this one merged, but would like them listed in a section in the first comment as a list of items to finish completing.
Thank you Patrick! I added the deferred items to the description. |
YulNaumenko
reviewed
Oct 11, 2021
| const showLegacyTooltip = | ||
| itemConfig?.isLegacy && | ||
| // TODO: Remove when applications are certified | ||
| ((ENABLE_NEW_SN_ITSM_CONNECTOR && item.actionTypeId === '.servicenow') || |
There was a problem hiding this comment.
For now this approach should be OK, but we might think about the more generic way of exposing this check. But this is on the alerting team to provide this ability.
6 tasks
jonathan-buttner
approved these changes
Oct 11, 2021
| * of our ServiceNow application | ||
| */ | ||
|
|
||
| describe('config', () => { |
There was a problem hiding this comment.
nit: do we need these unit tests? The configurations seem to be only objects and not functions.
There was a problem hiding this comment.
The only reason I put these tests there is because the configuration is very important (a wrong table name will make the whole connector impossible to use). I wanted in case of a change the developer to be notified somehow and think twice about the change. Do you think I can do that otherwise? With TS for example?
There was a problem hiding this comment.
Gotcha, leaving them is fine.
| * 2.0. | ||
| */ | ||
|
|
||
| // TODO: Remove when Elastic for ITSM is published. |
There was a problem hiding this comment.
Christos, is the plan to set these to false before feature freeze?
| * The user can not use a legacy connector | ||
| */ | ||
|
|
||
| export const connectorValidator = ( |
There was a problem hiding this comment.
Are we using the result message Deprecated connector anywhere? We probably should translate it right?
If not, I think it'd probably be clearer if we renamed this to something like isLegacyConnector and return true or false.
We can fix this in the follow up PR.
There was a problem hiding this comment.
Each connector validator is used by the Kibana form lib. To indicate an error you have to return an object with a message:
export interface ValidationError<T = string> {
message: string;
code?: T;
validationType?: string;
__isBlocking__?: boolean;
[key: string]: any;
}
Normally this text will be shown under the field as a form error. In a previous PR, the design team did not want the message to be shown because we already show a callout with a message. Do you want me to translate it eitherway?
There was a problem hiding this comment.
Gotcha, na I don't think we need to translate it if we're not going to show it. I would maybe put a comment as to why it isn't translated.
| const abortCtrl = new AbortController(); | ||
| fetchMock.mockResolvedValueOnce(applicationInfoResponse); | ||
|
|
||
| try { |
There was a problem hiding this comment.
If this test doesn't throw then it will succeed. It might be better to wrap the await call in an expect if the test should always throw
something like:
expect.assertions(1)
await expect(getAppInfo({...}))
.rejects
.toThrow();
| } | ||
| }); | ||
|
|
||
| it('returns an error when parsing the json fails', async () => { |
There was a problem hiding this comment.
Hmm I don't see any difference between this test and the one above. Do we need it? Should we add in a mockImplementation for a json parse failure?
There was a problem hiding this comment.
Bad copy paste from Swimlane 😄
| }); | ||
|
|
||
| test('should return true if there is failure status', async () => { | ||
| // @ts-expect-error |
There was a problem hiding this comment.
super nit: it might be more clear to cast {status: 'failure'} as the type we need as it takes me a second to figure out where the error would be coming from.
There was a problem hiding this comment.
I think it is better to use @ts-expect-error from casting because if there is a change in the interface and there is no error, TS will report that @ts-expect-error was not necessary. What do you think? Is there any other way I can make the test clearer?
There was a problem hiding this comment.
Yeah good point. I guess the reason that it wasn't clear to me is that when reviewing the code, if I only look at the tests, I don't immediately know why there will be a ts error. I had to go look at the function's implementation to figure it out. It's in the next file though so not a huge deal haha.
|
|
||
| export const choicesToEuiOptions = (choices: Choice[]): EuiSelectOption[] => | ||
| choices.map((choice) => ({ value: choice.value, text: choice.label })); | ||
|
|
||
| export const isRESTApiError = (res: AppInfo | RESTApiError): res is RESTApiError => |
There was a problem hiding this comment.
We might be able to avoid the expect ts error in the tests by making res: unknown here and doing additional checks for error.message and error.details
There was a problem hiding this comment.
I am not against it. What is the benefit of not having strict types of what we expect? Or what is the benefit of having it in the first place 😋 ?
There was a problem hiding this comment.
Yeah strict types is probably better haha.
| (key: string, value: string) => editActionSecrets(key, value), | ||
| [editActionSecrets] | ||
| const afterActionConnectorSave = useCallback(async () => { | ||
| // TODO: Implement |
There was a problem hiding this comment.
Do we need to implement this?
| export const isRESTApiError = (res: AppInfo | RESTApiError): res is RESTApiError => | ||
| (res as RESTApiError).error != null || (res as RESTApiError).status === 'failure'; | ||
|
|
||
| export const isFieldInvalid = ( |
There was a problem hiding this comment.
Will a field always been invalid if it is undefined? Or is that only true for required fields?
There was a problem hiding this comment.
Before the component mounts the validation runs. The fields are undefined and for that reason, the error is not empty. We do not want to show an error for this scenario. If the user "touches" the text field then the value will be an empty string (aka not undefined) or it will be what the user typed. In this scenario, we want to show an error because the user interacted with the form.
| }) | ||
| ); | ||
|
|
||
| try { |
There was a problem hiding this comment.
nit: might be clearer if we switch this to use the expect.assertions(#) and await expect(...).rejects.toThrow(...)
There was a problem hiding this comment.
You are right! A much better pattern.
For some reason, I cannot answer below your comment. This will remain true after FF so the QA can test the new functionality. If for some reason we are not certified until the last BC then we gonna set it to false. |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/data_frame_analytics/feature_importance·ts.machine learning data frame analytics total feature importance panel and decision path popover binary classification job should display the total feature importance in the results viewStandard OutStack TraceMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @cnasikas |
cnasikas
added
the
auto-backport
💔 Backport failed
To backport manually run: |
cnasikas
added a commit
that referenced
this pull request
Oct 13, 2021
Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # x-pack/plugins/actions/server/saved_objects/actions_migrations.ts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR is the implementation of https://github.com/elastic/security-team/issues/1454 and https://github.com/elastic/security-team/issues/1769.
doc link: https://kibana_105440.docs-preview.app.elstc.co/guide/en/kibana/master/action-types.html
Deferring items
isLegacytoisDepreactedand make it global. I think we need to discuss offline it in more detail to understand your needs.params.context-typeto see if the response is an HTML page and maybe return a better message.Features:
New connectors
Old connectors
Alerts
Cases
Screenshots:
Connector:
Cases:
Checklist
Delete any items that are not applicable to this PR.
For maintainers