-
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
[Osquery] Fix osquery response actions validations #144994
[Osquery] Fix osquery response actions validations #144994
Conversation
tomsonpl
commented
Nov 10, 2022
•
edited by kibanamachine
Loading
edited by kibanamachine
- Unified the logic of showing Response Actions errors in Actions step ✅
- The custom validation of response actions is now connected to the wrapper (actions rule step form). Since we do not know yet what the feedback for this feature is, I decided to go with rather a quick than generic approach, so there is no ''config'' for each response action. ✅
- Fix wrong data update triggered by ''validation'' ✅
- Open ecsMapping field if ecs isn't empty ✅
- fix Osquery response action rerender issue ✅
- fix backgroundColor issue ✅
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…available' into fix/osquery-response-action-not-available
…available' into fix/osquery-response-action-not-available
@elasticmachine merge upstream |
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.
@tomsonpl Rules Area LGTM
I left comments outside our area's responsibility so it may help to reason about improving the readability.
cy.getBySel('response-actions-list-item-0').within(() => { | ||
cy.contains('Query is a required field'); | ||
inputQuery('select * from uptime1'); | ||
cy.wait(1000); // wait for the validation to trigger - cypress is way faster than users ;) |
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's unclear what cy.wait(1000);
gives here. Why it should wait and why 1000ms? Cypress will anyway be able to click on osquery button.
cy.wait(1000); // wait for the validation to trigger - cypress is way faster than users ;) | ||
}); | ||
|
||
cy.getBySel('.osquery-ResponseActionTypeSelectOption').click(); |
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'd rather move the selector to a shared file and use cy.get(OSQUERY_BTN)
here.
It looks as a class name.osquery-ResponseActionTypeSelectOption
though it's an data-test
attribute. So it looks safe to remove the leading period.
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 idea, thank you!
|
||
cy.getBySel('.osquery-ResponseActionTypeSelectOption').click(); | ||
|
||
cy.getBySel('response-actions-list-item-1').within(() => { |
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'd rather move the selector to a shared file and use cy.get(RESPONSE_ACTIONS_ITEM1)
here or something like that.
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.
👍
cy.contains('select * from uptime'); | ||
cy.contains('Log message optimized for viewing in a log viewer'); | ||
cy.contains('Days of uptime'); | ||
}); | ||
}); |
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.
The test is bulky and it's hard to read it due to that. The name doesn't fully reflect what's going on inside.
One of the options to improve it is to extract the repeating content into a separate helper functions with clear names.
Another approach is to split it into smaller tests but it's controversial since e2e tests aren't so light as unit tests and can have enough number of assertions.
@@ -68,6 +68,14 @@ const LiveQueryQueryFieldComponent: React.FC<LiveQueryQueryFieldProps> = ({ | |||
defaultValue: '', | |||
}); | |||
|
|||
useEffect(() => { | |||
if (!isEmpty(ecsMapping)) { |
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.
Avoid nested if
s since it reduces readability, it can be transformed into if (!isEmpty(ecsMapping) && advancedContentState === 'closed') {
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.
👍
}; | ||
} | ||
}, [errors, handleSubmit, isValid, item.id, onSubmit, ref, watchedValues]); | ||
lastErrors.current = formState.errors; |
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.
Why is it required to have the lastErrors
here as a reference. I don't see where lastErrors
is 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.
edit: ouch, it's not that variable. Let me check more carefully :)
It is used above in line 756
if (!deepEqual(latestErrors.current, formState.errors.ecsMappingArray)) {
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.
removed 👍
const { setErrors, clearErrors, value, setValue } = field; | ||
const { osquery } = useKibana().services; | ||
|
||
const OsqueryForm = useMemo( |
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.
Does it give any performance improvements?
} | ||
|
||
export const ResponseActionTypeForm = React.memo((props: IProps) => { | ||
const { item, onDeleteAction, formRef } = props; | ||
const StyledEuiAccordion = styled(EuiAccordion)` |
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.
EUI exposes css
prop for simple inline styling. It looks like a good fit here.
[key: string]: () => Promise<boolean>; | ||
}; | ||
} | ||
const FieldErrorsContainer = styled.div` |
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.
As far as I know EUI uses emotion under the hood so it's better to avoid adding more styled-components
dependencies if we decide to get rid of it. There is a css
helper which can be used from emotion
for inline modifications
import { css } from '@emotion/react';
const withoutBottomMargin = css`
margin-bottom: 0;
`;
...
<p css={withoutBottomMargin}
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 hint, thank you!
|
||
useEffect(() => { | ||
setUIFieldErrors(() => { | ||
const fieldErrors = reduce<string[], Array<{ type: string; errors: string[] }>>( |
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.
Overall reduce
may look convenient but it makes the code bulky so it's harder to read. As a readability improvement it can be refactored to use for loops.
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 sure about it, also not a fan of using for loops
, but will give it a shot and try to experiment with it. Thanks!
@maximpn Big thanks for your comments, I'll go through all of them very carefully and make the improvements 👍 Thank you for the feedback and your time! |
@elasticmachine merge upstream |
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.
LGTM
return { | ||
code: 'ERR_FIELD_MISSING', | ||
path, | ||
message: '**ResponseActions:**\n Osquery Response Action is not available.\n ', |
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.
i18n
, title/header should be separate from the actual 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.
This piece of code does not exist anymore 👍
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.
Investigations codeowners LGTM!
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @tomsonpl |
(cherry picked from commit 1cb49bb)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…146391) # Backport This will backport the following commits from `main` to `8.6`: - [[Osquery] Fix osquery response actions validations (#144994)](#144994) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Tomasz Ciecierski","email":"tomasz.ciecierski@elastic.co"},"sourceCommit":{"committedDate":"2022-11-28T13:37:14Z","message":"[Osquery] Fix osquery response actions validations (#144994)","sha":"1cb49bb4352b0c175c908727c6bd9e67dc73c70d","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Asset Management","Feature:Osquery","v8.6.0","v8.7.0"],"number":144994,"url":"https://github.com/elastic/kibana/pull/144994","mergeCommit":{"message":"[Osquery] Fix osquery response actions validations (#144994)","sha":"1cb49bb4352b0c175c908727c6bd9e67dc73c70d"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/144994","number":144994,"mergeCommit":{"message":"[Osquery] Fix osquery response actions validations (#144994)","sha":"1cb49bb4352b0c175c908727c6bd9e67dc73c70d"}}]}] BACKPORT--> Co-authored-by: Tomasz Ciecierski <tomasz.ciecierski@elastic.co>