[New platform] Restrict import from core&plugin internals for js files #33697

mshustov · 2019-03-22T10:39:36Z

import/no-restricted-paths supports restrictions by zone mechanism.

Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define from: plugins/*/server/ the rule will report all relative imports in the same folder as well. To fix this problem I added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder.

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-03-22T10:39:38Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-03-22T11:33:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define 'from: plugins/*/server/' the rule will report all relative imports in the same folder as well. To fix this problem we added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder.

mshustov · 2019-03-25T10:17:30Z

packages/kbn-eslint-plugin-eslint/rules/__tests__/no_restricted_paths.js

+  return path.join(__dirname, 'files', relativePath);
+}
+
+ruleTester.run('@kbn/eslint/no-restricted-paths', rule, {


@spalger I cannot see "test" script in https://github.com/elastic/kibana/blob/master/packages/kbn-eslint-plugin-eslint/package.json
how we run tests for the package? I did use `node path/to/test' to run tests manually

node scripts/mocha packages/kbn-eslint-plugin-eslint/rules/__tests__/no_restricted_paths.js

elasticmachine · 2019-03-25T10:59:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-25T12:13:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

packages/kbn-eslint-plugin-eslint/package.json

spalger · 2019-03-25T23:20:23Z

packages/kbn-eslint-plugin-eslint/rules/no_restricted_paths.js

+  create(context) {
+    const options = context.options[0] || {};
+    const zones = options.zones || [];
+    const basePath = process.cwd();


Can we make this configurable? We use .js files for eslint config so we can provide the absolute root for all files, or even require that zone.target/zone.target are absolute.

.eslintrc.js

spalger · 2019-03-25T23:39:17Z

.eslintrc.js

+        zones: [
+          {
+            // when tslint is removed we will check *.ts files as well
+            target: './src/legacy/.*js$',


I'm not opposed to limiting to .js files, but do you think it's necessary? ESLint will only process the files .js files, and this will automatically apply to .ts files as soon as we get them in here without this limitation.

didn't want to force #33826 to comment/fix all problem places in *.ts files.

We're going to limit all existing rules to .js files in that PR, so I wouldn't worry about it.

spalger · 2019-03-25T23:40:04Z

packages/kbn-eslint-plugin-eslint/rules/no_restricted_paths.js

@@ -0,0 +1,149 @@
+/* eslint-disable @kbn/eslint/require-license-header */
+/* @notice


Since we don't ship this code in the distributable I don't think we need to include the @notice, do you agree @epixa?

Hm, I'd have to reacquaint myself with how we handle that annotation. I don't think it makes sense to ship this package's license info in our distributions' notice file. I do still think we should be automatically checking it's license, albeit against our dev-acceptable list. In general, I'd rather ship a dependency's info in our notice file by accident than accidentally leave out a dependency we should have included.

This tag doesn't cause the licence to be checked, that's something we do "manually" by committing the code to the repo. This tag is only for generating the notice file in the build, so I think we can get rid of it.

epixa · 2019-03-25T23:57:45Z

I think I saw a reason for this, but I can't track it down: can we contribute the change upstream?

mshustov · 2019-03-26T14:40:17Z

@epixa I was going but realized it could take some time to have it merged. At least there are several PRs are waiting for CR import-js/eslint-plugin-import#1238

mshustov · 2019-03-26T20:00:27Z

@spalger switched to micromatch. it has multimatch out-of-the-box and faster than minimatch https://gist.github.com/jonschlinkert/96212e11d128fc659d12.
as you asked in #33697 (comment)
not sure if we should keep reference to import/no-restricted-paths, there is another implementation and comment can be misleading. should I remove it ?

elasticmachine · 2019-03-26T20:45:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2019-03-26T23:55:25Z

packages/kbn-eslint-plugin-eslint/rules/no_restricted_paths.js

+}
+
+function resolveBasePath(basePath) {
+  return path.isAbsolute(basePath) ? basePath : path.relative(process.cwd(), basePath);


path.resolve(process.cwd(), basePath) is going to return a relative path... I don't think that's what you're trying to do here.

spalger · 2019-03-26T23:56:32Z

packages/kbn-eslint-plugin-eslint/rules/no_restricted_paths.js

+  create(context) {
+    const options = context.options[0] || {};
+    const zones = options.zones || [];
+    const basePath = options.basePath ? resolveBasePath(options.basePath) : process.cwd();


Why not just require options.basePath? I think it would make the behavior of this rule more explicit and easier to reason about.

const basePath = options.basePath; if (!basePath || !isAboslute(basePath)) { throw new Error('basePath option must be specified and must be absolute') }

elasticmachine · 2019-03-27T10:10:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

LGTM

@notice

elastic#33697) * restrict import from core&plugin internals * Fork import/no-restricted-paths and add allowSameFolder option Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define 'from: plugins/*/server/' the rule will report all relative imports in the same folder as well. To fix this problem we added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder. * update notices * add basePath option * support glob pattern instead of reagexp * remove @notice, make basePath required

@notice

#33697) (#34139) * restrict import from core&plugin internals * Fork import/no-restricted-paths and add allowSameFolder option Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define 'from: plugins/*/server/' the rule will report all relative imports in the same folder as well. To fix this problem we added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder. * update notices * add basePath option * support glob pattern instead of reagexp * remove @notice, make basePath required

mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc non-issue Indicates to automation that a pull request should not appear in the release notes Feature:New Platform v7.2.0 labels Mar 22, 2019

mshustov added 2 commits March 25, 2019 09:23

restrict import from core&plugin internals

5e8cde7

mshustov force-pushed the issue-33444-restrict-imports-eslint branch from 50ea2d2 to 5e1510a Compare March 25, 2019 10:11

mshustov added release_note:skip Skip the PR/issue when compiling release notes review and removed non-issue Indicates to automation that a pull request should not appear in the release notes labels Mar 25, 2019

mshustov marked this pull request as ready for review March 25, 2019 10:14

mshustov requested review from a team March 25, 2019 10:14

mshustov commented Mar 25, 2019

View reviewed changes

update notices

f0a89b6

spalger reviewed Mar 25, 2019

View reviewed changes

mshustov added 3 commits March 26, 2019 16:18

add basePath option

8c0d2b5

support glob pattern instead of reagexp

cdaf22e

Merge branch 'master' into issue-33444-restrict-imports-eslint

6c9c965

spalger reviewed Mar 26, 2019

View reviewed changes

mshustov added 2 commits March 27, 2019 10:12

remove @notice, make basePath required

253a96f

Merge branch 'master' into issue-33444-restrict-imports-eslint

328c122

mshustov requested a review from spalger March 28, 2019 12:29

rudolf approved these changes Mar 28, 2019

View reviewed changes

spalger approved these changes Mar 28, 2019

View reviewed changes

mshustov merged commit 2489180 into elastic:master Mar 29, 2019

mshustov deleted the issue-33444-restrict-imports-eslint branch March 29, 2019 07:48

mshustov mentioned this pull request Mar 29, 2019

[7.x] [New platform] Restrict import from core&plugin internals for js files (#33697) #34139

Merged

mshustov mentioned this pull request Mar 29, 2019

Lint for cross-system imports only happening to root of systems #33444

Closed

mshustov removed the review label Apr 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New platform] Restrict import from core&plugin internals for js files #33697

[New platform] Restrict import from core&plugin internals for js files #33697

mshustov commented Mar 22, 2019 •

edited

elasticmachine commented Mar 22, 2019

elasticmachine commented Mar 22, 2019

mshustov Mar 25, 2019

spalger Mar 25, 2019 •

edited

elasticmachine commented Mar 25, 2019

elasticmachine commented Mar 25, 2019

spalger Mar 25, 2019

spalger Mar 25, 2019

mshustov Mar 26, 2019

spalger Mar 26, 2019

spalger Mar 25, 2019

epixa Mar 25, 2019

spalger Mar 26, 2019

epixa commented Mar 25, 2019

mshustov commented Mar 26, 2019

mshustov commented Mar 26, 2019 •

edited

elasticmachine commented Mar 26, 2019

spalger Mar 26, 2019

spalger Mar 26, 2019

elasticmachine commented Mar 27, 2019

spalger left a comment

		@@ -0,0 +1,149 @@
		/* eslint-disable @kbn/eslint/require-license-header */
		/* @notice

[New platform] Restrict import from core&plugin internals for js files #33697

[New platform] Restrict import from core&plugin internals for js files #33697

Conversation

mshustov commented Mar 22, 2019 • edited

Checklist

For maintainers

elasticmachine commented Mar 22, 2019

elasticmachine commented Mar 22, 2019

💔 Build Failed

Choose a reason for hiding this comment

spalger Mar 25, 2019 • edited

Choose a reason for hiding this comment

elasticmachine commented Mar 25, 2019

💔 Build Failed

elasticmachine commented Mar 25, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa commented Mar 25, 2019

mshustov commented Mar 26, 2019

mshustov commented Mar 26, 2019 • edited

elasticmachine commented Mar 26, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 27, 2019

💚 Build Succeeded

spalger left a comment

Choose a reason for hiding this comment

mshustov commented Mar 22, 2019 •

edited

spalger Mar 25, 2019 •

edited

mshustov commented Mar 26, 2019 •

edited