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

Prevents importing of public code into server #67149

Merged
merged 13 commits into from
Jun 10, 2020

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented May 21, 2020

We should not be allowing importing of public into server. Any shared code should reside in a common directory. After #66506, this will not even be possible as we will no longer be transpiling public code into commonjs.

Relates to #66506

We should not be allowing importing of public into server. Any shared code should reside in a common directory. After elastic#66506, this will not even be possible as we will no longer be transpiling public code into commonjs.

Blocks elastic#66506

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label May 21, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
.eslintrc.js Outdated
@@ -197,6 +197,11 @@ module.exports = {
from: ['src/core/utils/**/*'],
errorMessage: `Plugins may only import from src/core/server and src/core/public.`,
},
{
target: ['(src|x-pack)/plugins/*/server/**/*', '!x-pack/plugins/apm/**/*'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

APM is a less than straightforward update and I would rather not make assumptions for their desired organization. I will follow-up with an issue for the team to resolve.

Copy link
Member

@sorenlouv sorenlouv May 21, 2020

Choose a reason for hiding this comment

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

I think what you are hitting is caused by the server side types consumed by the client.
Types are inferred from server-side routes and can therefore not be moved to common.

I expect we can resolve this when typescript is updated to 3.8 and we can use type only imports.

Copy link
Member

Choose a reason for hiding this comment

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

tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request May 21, 2020
Introduced in elastic#66432 and I have a more thorough PR to prevent imports in
server from public in elastic#67149

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley tylersmalley added backport:skip This commit does not require backporting Team:Operations Team label for Operations Team v7.8.0 v8.0.0 labels May 21, 2020
@elasticmachine
Copy link
Contributor

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

@tylersmalley tylersmalley marked this pull request as ready for review May 21, 2020 13:40
@tylersmalley tylersmalley requested review from a team as code owners May 21, 2020 13:40
@tylersmalley tylersmalley added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This commit does not require backporting labels May 21, 2020
Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

LGTM

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

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

SIEM and endpoint 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.

@tylersmalley 👍 I'm onboard with the intention behind this change. However, one of ES UI's organizing principles is to organize code inside of __packages_do_not_import__ and then re-export from public, server, and common directories. Can we make a couple adjustments that will both follow this principle and fulfill the goal of preventing public code from being imported into server code?

Here's what I think will work:

  1. Moving all the authorization code back into its original state inside of __packages_do_not_import__.
  2. Changing the exports from public/authorization to be explicit. None of these modules are usable on the server, so common isn't the best place for them.
export {
  WithPrivileges,
  NotAuthorizedSection,
  AuthorizationProvider,
  AuthorizationContext,
  SectionError,
  Error,
  useAuthorizationContext,
} from '../../__packages_do_not_import__/authorization';
  1. Changing the exports from common/index.ts to consist of the types only, since they're the only things that are usable on both client and server:
export {
  Privileges,
  MissingPrivileges,
} from '../../__packages_do_not_import__/authorization';

Tyler Smalley added 2 commits May 21, 2020 13:18
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
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 LGTM! Didn't test locally. Thanks for making the change I suggested!

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM
code review

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley
Copy link
Contributor Author

@elasticmachine merge upstream

@tylersmalley
Copy link
Contributor Author

@elasticmachine merge upstream

elasticmachine and others added 4 commits May 25, 2020 06:09
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley tylersmalley requested a review from a team as a code owner May 25, 2020 20:45
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Usage collection changes (src/plugins/usage_collection/server/report/store_report.test.ts) LGTM!

@tylersmalley
Copy link
Contributor Author

@elastic/kibana-app and @elastic/siem - can I get a codeowners review here?

@rylnd
Copy link
Contributor

rylnd commented May 26, 2020

pinging @elastic/endpoint-app-team as they own the conflicting files.

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

@elastic/kibana-app-arch LGTM

Tyler Smalley added 2 commits June 9, 2020 13:06
@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 8a5a7c3 into elastic:master Jun 10, 2020
tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Jun 10, 2020
We should not be allowing importing of public into server. Any shared code should reside in a common directory. After elastic#66506, this will not even be possible as we will no longer be transpiling public code into commonjs.

Blocks elastic#66506

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
tylersmalley pushed a commit that referenced this pull request Jun 10, 2020
We should not be allowing importing of public into server. Any shared code should reside in a common directory. After #66506, this will not even be possible as we will no longer be transpiling public code into commonjs.

Blocks #66506

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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:APM All issues that need APM UI Team support Team:Operations Team label for Operations Team v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet