Skip to content
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

Btsymbala/registered av #81910

Merged
merged 23 commits into from
Nov 12, 2020
Merged

Conversation

efreeti
Copy link
Contributor

@efreeti efreeti commented Oct 28, 2020

Summary

Added Antivirus registration switch in the policy form. Additionally fixed a bit layout and text styling as per discussion with Bonnie. Fixed the mobile view.

antivirus_registration

Checklist

@efreeti efreeti self-assigned this Oct 30, 2020
@efreeti efreeti added Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution release_note:enhancement v7.11.0 v8.0.0 and removed release_note:skip Skip the PR/issue when compiling release notes labels Oct 30, 2020
@efreeti efreeti marked this pull request as ready for review October 30, 2020 20:48
@efreeti efreeti requested review from a team as code owners October 30, 2020 20:48
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@parkiino : would you take a quick look as well since you are most familiar with the policy edit forms 😬

…bala/registered-av

� Conflicts:
�	x-pack/plugins/security_solution/public/management/pages/policy/view/policy_forms/protections/malware.tsx
storiesOf('PolicyDetails/ConfigForm', module)
.add('One OS', () => {
return (
<ConfigForm type="Type 1" supportedOss={['windows']}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use the os enums you made here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you refer to os.ts file then it contains only type definitions, unfortunately literal types can not be used as keys/symbols directly. I plan to later revisit this and add some constant to keep the keys.

@@ -126,5 +148,26 @@ export const policyDetailsReducer: ImmutableReducer<PolicyDetailsState, AppActio
return newState;
}

if (action.type === 'userChangedAntivirusRegistration') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you created a separate action for antivirus registration specifically? If I am reading this right, it seems like the antivirus_registration is just another key within the policy config. The action userChangedPolicyConfig should be able to do the same thing. ex) https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/management/pages/policy/view/policy_forms/protections/malware.tsx#L53

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will bring this up during next week office hours. General thought was to avoid components being too smart and knowing how to update properties in policy config and rather keep the logic out (specifically move it to redux). What are your thoughts on this?

I noticed that due to this logic living in component we clone policy twice and have some ts-ignore directives because the code violates immutability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh okie, i can see the value in moving logic into redux, i guess im just wondering if its possible to make it more robust since i feel like policy details will only get bigger in the future?

@efreeti
Copy link
Contributor Author

efreeti commented Nov 11, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2068 2071 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.8MB 7.8MB +549.0B

Distributable file count

id before after diff
default 42778 42779 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@efreeti efreeti merged commit 58ad7ec into elastic:master Nov 12, 2020
@efreeti efreeti deleted the btsymbala/registered-av branch November 12, 2020 14:54
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
* master:
  [Ingest Manager] Lift up registry/{stream,extract} functions (elastic#83239)
  [Reporting] Move "common" types and constants to allow cross-plugin integration (elastic#83198)
  [Lens] Add suffix formatter (elastic#82852)
  [App Search] Version documentation links (elastic#83245)
  Use saved object references for dashboard drilldowns (elastic#82602)
  Btsymbala/registered av (elastic#81910)
  [APM] Errors table for service overview (elastic#83065)
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 81910 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 16, 2020
efreeti added a commit to efreeti/kibana that referenced this pull request Nov 16, 2020
* Moved out type for OperatingSystem and moved OS translations one level higher.

* Changed the translation to be consistent between trusted apps and policy.

* Unified translations of OS types between trusted apps and policy.

* Removed unused types.

* Added registered AV form section.

* Changed the property structure to match the format expected by endpoint.

* Fixed the visual alignment of titles in the form and added responsiveness.

* Updated snapshots.

* Moved out type for OperatingSystem and moved OS translations one level higher.

* Added config form heading component.

* Cleaned up translations.

* Fixed type error with initialization.

* Fixed error in trusted app creation form test.

* Removed the guard for now in favour of better initialization.

* Fixed the store test.

* Fixing functional test data.

* Added functional test config option to account for a custom header within security app.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
efreeti added a commit that referenced this pull request Nov 17, 2020
* Moved out type for OperatingSystem and moved OS translations one level higher.

* Changed the translation to be consistent between trusted apps and policy.

* Unified translations of OS types between trusted apps and policy.

* Removed unused types.

* Added registered AV form section.

* Changed the property structure to match the format expected by endpoint.

* Fixed the visual alignment of titles in the form and added responsiveness.

* Updated snapshots.

* Moved out type for OperatingSystem and moved OS translations one level higher.

* Added config form heading component.

* Cleaned up translations.

* Fixed type error with initialization.

* Fixed error in trusted app creation form test.

* Removed the guard for now in favour of better initialization.

* Fixed the store test.

* Fixing functional test data.

* Added functional test config option to account for a custom header within security app.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature release_note:enhancement Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants