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

[eslint] no_restricted_paths config cleanup #63741

Merged
merged 4 commits into from
Apr 23, 2020

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented Apr 16, 2020

Refactor/fixes of the no_restricted_paths rule config for imports of core.

In no_restricted_paths we resolve relative imports with eslint-module-utils/resolve which resolves to the full filesystem path. So, to support relative and absolute imports from the src alias we need to define both the directory and the index including file extension. An alternative could be to change how we handle this in no_restricted_paths by checking if it's a node_module or from source.

This rule was handling both core imports, as well as imports from other plugins. Imports from other plugins are being used much more liberally allowed through the exceptions in tests. I choose to break these up, removing this exception for tests for core imports.

Fixes:

  • Absolute imports of src/core/server/mocks were not allowed in src. This was not an issue in x-pack due to the target excluding !x-pack/**/*.test.* and !x-pack/test/**/*.
  • Non-top-level public and server imports were allowed from X-Pack tests to the previously mentioned exclusion.

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Apr 16, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@tylersmalley tylersmalley changed the title [eslint] Removes extensions from no_restricted_paths config [eslint] Allows for relative and absolute import into core for no_restricted_paths config Apr 16, 2020
@tylersmalley tylersmalley force-pushed the eslint-ts branch 2 times, most recently from c746917 to 1f0484b Compare April 17, 2020 02:04
@tylersmalley tylersmalley changed the title [eslint] Allows for relative and absolute import into core for no_restricted_paths config [eslint] no_restricted_paths config cleanup Apr 17, 2020
@tylersmalley tylersmalley force-pushed the eslint-ts branch 4 times, most recently from 12d95a3 to 06695cd Compare April 17, 2020 14:35
Major cleanup of the no_restricted_paths rule for imports of core.

For relative imports, we use eslint-module-utils/resolve which resolves
to the full filesystem path. So, to support relative and absolute
imports from the src alias we need to define both the directory and the
index including file extension.

This rule was handling both core imports, as well as imports from other
plugins. Imports from other plugins are being used much more liberally
allowed through the exceptions in tests. I choose to break these up,
removing this exception for tests for core imports.

Fixes:
Absolute imports of src/core/server/mocks were not allowed in src. This
was not an issue in x-pack due to the target excluding
!x-pack/**/*.test.* and !x-pack/test/**/*.

Non-top-level public and server imports were allowed from X-Pack tests
to the previously mentioned exclusion.

Signed-off-by: Tyler Smalley <[email protected]>
@@ -143,6 +143,7 @@ export {
export {
HttpHeadersInit,
HttpRequestInit,
HttpFetchError,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we OK with this? It's being requested from tests, so either we can add src/core/public/http as an exception to the ESLint rule, or we can exempt tests from requiring top level imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @elastic/kibana-platform

@tylersmalley tylersmalley marked this pull request as ready for review April 20, 2020 16:49
@tylersmalley tylersmalley requested a review from a team as a code owner April 20, 2020 16:49
@tylersmalley tylersmalley requested a review from a team April 20, 2020 16:49
@tylersmalley tylersmalley requested review from a team as code owners April 20, 2020 16:49
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Ingest manager changes LGTM

@tylersmalley tylersmalley added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0 v8.0.0 labels Apr 20, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Need to test this locally still, but a couple comments

.eslintrc.js Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Security changes LGTM.

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

SIEM changes LGTM 💪

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Code changes to Upgrade Assistant LGTM, didn't test locally.

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting code LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@tylersmalley tylersmalley merged commit feed406 into elastic:master Apr 23, 2020
@tylersmalley
Copy link
Contributor Author

Waiting for #64306 to be merged before backporting to 7.x

gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 24, 2020
* master: (70 commits)
  KQL removes leading zero and breaks query (elastic#62748)
  [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193)
  [ML] Changes transforms wizard UI text (elastic#64150)
  [Alerting] change server log action type .log to .server-log in README (elastic#64124)
  [Metrics UI] Design Refresh: Inventory View, Episode 1 (elastic#64026)
  chore(NA): reduce siem bundle size using babel-plugin-transfor… (elastic#63269)
  chore(NA): use core-js instead of babel-polyfill on canvas sha… (elastic#63486)
  skip flaky suite (elastic#61173)
  skip flaky suite (elastic#62497)
  Renamed ilm policy for event log so it is not prefixed with dot (elastic#64262)
  [eslint] no_restricted_paths config cleanup (elastic#63741)
  Add Oil Rig Icon from @elastic/maki (elastic#64364)
  [Maps] Migrate Maps embeddables to NP (elastic#63976)
  [Ingest] Data streams list page (elastic#64134)
  chore(NA): add file-loader into jest moduleNameMapper (elastic#64330)
  [DOCS] Added images to automating report generation (elastic#64333)
  [SIEM][CASE] Api Integration Tests: Configuration (elastic#63948)
  Expose ability to check if API Keys are enabled (elastic#63454)
  [DOCS] Fixes formatting in alerting doc (elastic#64338)
  [data.search.aggs]: Create agg types function for terms agg. (elastic#63541)
  ...
tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Apr 24, 2020
Major cleanup of the no_restricted_paths rule for imports of core.

For relative imports, we use eslint-module-utils/resolve which resolves
to the full filesystem path. So, to support relative and absolute
imports from the src alias we need to define both the directory and the
index including file extension.

This rule was handling both core imports, as well as imports from other
plugins. Imports from other plugins are being used much more liberally
allowed through the exceptions in tests. I choose to break these up,
removing this exception for tests for core imports.

Fixes:
Absolute imports of src/core/server/mocks were not allowed in src. This
was not an issue in x-pack due to the target excluding
!x-pack/**/*.test.* and !x-pack/test/**/*.

Non-top-level public and server imports were allowed from X-Pack tests
to the previously mentioned exclusion.

Signed-off-by: Tyler Smalley <[email protected]>
tylersmalley pushed a commit that referenced this pull request Apr 24, 2020
Major cleanup of the no_restricted_paths rule for imports of core.

For relative imports, we use eslint-module-utils/resolve which resolves
to the full filesystem path. So, to support relative and absolute
imports from the src alias we need to define both the directory and the
index including file extension.

This rule was handling both core imports, as well as imports from other
plugins. Imports from other plugins are being used much more liberally
allowed through the exceptions in tests. I choose to break these up,
removing this exception for tests for core imports.

Fixes:
Absolute imports of src/core/server/mocks were not allowed in src. This
was not an issue in x-pack due to the target excluding
!x-pack/**/*.test.* and !x-pack/test/**/*.

Non-top-level public and server imports were allowed from X-Pack tests
to the previously mentioned exclusion.

Signed-off-by: Tyler Smalley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants