[SIEM] Low impact linter rules to start with #37137

FrankHassanabad · 2019-05-24T22:52:30Z

Summary

Low impact linter rules to help catch bugs for back-porting as well as moving into the future. We will slowly turn the warns into errors and fix the errors as we go along. This helps us avoid a "big bang" switch over approach to avoid back-port issues as we are porting bugs aggressively at the beginning.

Added a custom pattern detection rule for determining if the UI is trying to import anything from the server backend since that would cause web-pack front to pull in backend code. Also forbid the use of NodeJS imports on the front end.

Double checked that I was not duplicating rules from:

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-05-24T22:54:19Z

Pinging @elastic/secops

elasticmachine · 2019-05-24T23:51:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…we are going to fix right now

.eslintrc.js

spong · 2019-05-28T16:11:57Z

.eslintrc.js

+        // these rules cannot be turned on and tested at the moment until this issue is resolved:
+        // https://github.com/prettier/prettier-eslint/issues/201
+        // '@typescript-eslint/await-thenable': 'error',
+        // '@typescript-eslint/no-non-null-assertion': 'error'


Looking forward to this one! :)

spong · 2019-05-28T16:16:28Z

.eslintrc.js

+        // These will be turned on after bug fixes are mostly completed and we can
+        // run a fix-lint
+        /*
+        'import/order': [


Excited to get this back in as well. This is going to be a big change though, so I think holding off for now is 👍

spong · 2019-05-28T16:19:38Z

.eslintrc.js

+        // This style will be turned on after most bugs are fixed
+        // 'prefer-template': 'warn',
+        // This style will be turned on after most bugs are fixed
+        // quotes: ['warn', 'single', { avoidEscape: true }],


There've been plenty of PR comments for this one. ++ for this rule.

Soon for ones such as that one.

spong · 2019-05-28T16:33:16Z

.eslintrc.js

+        'react/void-dom-elements-no-children': 'error',
+        'react/jsx-boolean-value': ['error', 'warn'],
+        // will introduced after the other warns are fixed
+        // 'react/jsx-no-bind': 'error',


I think there's a strong argument for getting this rule on sooner than later from a perf standpoint. Should we open an issue for tracking to raise visibility? Turning it on showed 120 errors and it's pretty rampant around the DnD areas within the app.

spong · 2019-05-28T16:39:17Z

.eslintrc.js

+        'react/jsx-fragments': 'error',
+        'react/jsx-sort-default-props': 'error',
+        // might be introduced after the other warns are fixed
+        // 'react/jsx-sort-props': 'error',


From the existing patterns in the codebase, I think the team would be happy to have this enabled, albeit with the callbacksLast and reservedFirst options enabled as well.

spong · 2019-05-28T16:45:41Z

x-pack/plugins/siem/public/components/url_state/index.tsx

@@ -57,6 +57,7 @@ type KeyUrlState = keyof UrlState;

 interface UrlStateProps {
  indexPattern: StaticIndexPattern;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any


Good news -- I think this one is getting cleaned up here: https://github.com/elastic/kibana/pull/36954/files#diff-8fd82466ad8233a23f8deedd497d9058R58

elasticmachine · 2019-05-28T16:57:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spong

Checked out, ran linter locally and received 17 warnings as expected. Thanks for taking the time to curate these rules and also creating issues for tracking the high-impact rules. I think having the heavy-offenders commented out and turning them on + bugfixing one at a time is a good strategy for getting these enabled and the codebase cleaned up as we move to release.

I've left a few comments, but no changes necessary. Only addition I think would be to create a ticket for tracking the enabling of 'react/jsx-no-bind' as that should result in measurable performance gains with the number of errors that we have for it in our project. Anyway, LGTM! 👍

## Summary Low impact linter rules to help catch bugs for back-porting as well as moving into the future. We will slowly turn the warns into errors and fix the errors as we go along. This helps us avoid a "big bang" switch over approach to avoid back-port issues as we are porting bugs aggressively at the beginning. Added a custom pattern detection rule for determining if the UI is trying to import anything from the server backend since that would cause web-pack front to pull in backend code. Also forbid the use of NodeJS imports on the front end. Double checked that I was not duplicating rules from: * https://github.com/elastic/kibana/blob/master/packages/eslint-config-kibana/typescript.js * https://github.com/elastic/kibana/blob/master/packages/eslint-config-kibana/javascript.js ### Checklist Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR. ~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~ ~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~ ~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~ ~- [ ] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios~ ~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~ ### For maintainers ~- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~ ~- [ ] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~

FrankHassanabad added 2 commits May 24, 2019 16:43

Low impact linter rules to start with

5f4171c

Fixed to be less of an impact

53d0b54

FrankHassanabad added v8.0.0 v7.3.0 Team:SIEM labels May 24, 2019

FrankHassanabad self-assigned this May 24, 2019

FrankHassanabad changed the title ~~Low impact linter rules to start with~~ [SIEM] Low impact linter rules to start with May 24, 2019

FrankHassanabad added the release_note:skip Skip the PR/issue when compiling release notes label May 24, 2019

FrankHassanabad added 2 commits May 28, 2019 08:54

Merge branch 'master' into small-linter

6041cb6

Turned off most of the warn to get it down to only a few select bugs …

1b99d45

…we are going to fix right now

spong reviewed May 28, 2019

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

Fixed per code review comments

5930455

spong reviewed May 28, 2019

View reviewed changes

spong approved these changes May 28, 2019

View reviewed changes

FrankHassanabad merged commit 1cfad73 into elastic:master May 28, 2019

FrankHassanabad deleted the small-linter branch May 28, 2019 17:14

FrankHassanabad mentioned this pull request May 28, 2019

[7.x] [SIEM] Low impact linter rules to start with (#37137) #37253

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] Low impact linter rules to start with #37137

[SIEM] Low impact linter rules to start with #37137

FrankHassanabad commented May 24, 2019 •

edited

Loading

elasticmachine commented May 24, 2019

elasticmachine commented May 24, 2019

spong May 28, 2019

spong May 28, 2019

spong May 28, 2019

FrankHassanabad May 28, 2019

spong May 28, 2019 •

edited

Loading

spong May 28, 2019 •

edited

Loading

spong May 28, 2019

stephmilovic May 29, 2019

elasticmachine commented May 28, 2019

spong left a comment

[SIEM] Low impact linter rules to start with #37137

[SIEM] Low impact linter rules to start with #37137

Conversation

FrankHassanabad commented May 24, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented May 24, 2019

elasticmachine commented May 24, 2019

💚 Build Succeeded

spong May 28, 2019

Choose a reason for hiding this comment

spong May 28, 2019

Choose a reason for hiding this comment

spong May 28, 2019

Choose a reason for hiding this comment

FrankHassanabad May 28, 2019

Choose a reason for hiding this comment

spong May 28, 2019 • edited Loading

Choose a reason for hiding this comment

spong May 28, 2019 • edited Loading

Choose a reason for hiding this comment

spong May 28, 2019

Choose a reason for hiding this comment

stephmilovic May 29, 2019

Choose a reason for hiding this comment

elasticmachine commented May 28, 2019

💚 Build Succeeded

spong left a comment

Choose a reason for hiding this comment

FrankHassanabad commented May 24, 2019 •

edited

Loading

spong May 28, 2019 •

edited

Loading

spong May 28, 2019 •

edited

Loading