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.

Sign up for GitHub

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

Jump to bottom

[Resolver] generator uses setup_node_env #76422

Merged
merged 1 commit into from
Sep 1, 2020
Merged

[Resolver] generator uses setup_node_env #76422

merged 1 commit into from
Sep 1, 2020

Conversation

oatkiller
Copy link
Contributor

Summary

The Resolver generator script required users to install ts-node. It included its own tsconfig.json. This behavior isn't ideal. Furthermore #76082 breaks it.

With this PR the script no longer has its own tsconfig and no longer requires users to install tsnode. It does this by using the setup_node_env script.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@oatkiller oatkiller requested review from a team as code owners September 1, 2020 18:40
@oatkiller oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 1, 2020
@oatkiller
Copy link
Contributor Author

We should add an automated test that checks that this generator runs.

marshallmain
marshallmain approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

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

less dependencies is better

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

distributable file count

id value diff baseline
total 45532 +1 45531

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

@oatkiller oatkiller merged commit ac8e936 into elastic:master Sep 1, 2020
@oatkiller oatkiller deleted the resolver-generator-uses-setup-node-env branch September 1, 2020 21:39
@oatkiller oatkiller mentioned this pull request Sep 1, 2020
Closed
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Sep 1, 2020
[Resolver] generator uses setup_node_env (elastic#76422)
2046f51
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 2, 2020
@gmmorris
Merge branch 'master' into actions/webhook-remove-header
c9deb85 
* master: (223 commits)
  skip flaky suite (elastic#75724)
  [Reporting] Add functional test for Reports in non-default spaces (elastic#76053)
  [Enterprise Search] Fix various icons in dark mode (elastic#76430)
  skip flaky suite (elastic#76245)
  Add `auto` interval to histogram AggConfig (elastic#76001)
  [Resolver] generator uses setup_node_env (elastic#76422)
  [Ingest Manager] Support both zip & tar archives from Registry (elastic#76197)
  [Ingest Manager] Improve agent vs kibana version checks (elastic#76238)
  Manually building `KueryNode` for Fleet's routes (elastic#75693)
  remove dupe tinymath section (elastic#76093)
  Create APM issue template (elastic#76362)
  Delete unused file. (elastic#76386)
  [SECURITY_SOLUTION][ENDPOINT] Trusted Apps Create API (elastic#76178)
  [Detections Engine] Add Alert actions to the Timeline (elastic#73228)
  [Dashboard First] Library Notification (elastic#76122)
  [Maps] Add mvt support for ES doc sources  (elastic#75698)
  Add setHeaderActionMenu API to AppMountParameters (elastic#75422)
  [ML] Remove "Are you sure" from data frame analytics jobs (elastic#76214)
  [yarn] remove typings-tester, use @ts-expect-error (elastic#76341)
  [Reporting/CSV] Do not fail the job if scroll ID can not be cleared (elastic#76014)
  ...
paul-tavares
paul-tavares reviewed Sep 2, 2020
View reviewed changes
x-pack/plugins/security_solution/scripts/endpoint/resolver_generator.js
* you may not use this file except in compliance with the Elastic License.
*/

require('../../../../../src/setup_node_env');
Copy link
Contributor

Choose a reason for hiding this comment

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

@oatkiller I'm a bit late - sorry 😄
I was going to suggest that you also set permissions on this script to 755 and add the following shebang line to the top of file, so that we can just invoke it as a shell script:

#!/usr/bin/env node

I think you know this, but we (Endpoint Management) also use this script to load Endpoints 😬

(FYI: https://github.com/paul-tavares/kibana/blob/9873df8ee0ee88584db70debc3316126ba3205af/x-pack/plugins/security_solution/scripts/endpoint/load_trusted_apps.js#L1 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah I didn't know that. I copied the pattern from somewhere else in Kibana.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 3, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@oatkiller oatkiller mentioned this pull request Sep 8, 2020
Merged
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Sep 8, 2020
[Resolver] generator uses setup_node_env (elastic#76422)
e46ac2e 
# Conflicts:
#	x-pack/plugins/security_solution/package.json
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

oatkiller pushed a commit that referenced this pull request Sep 9, 2020
[Resolver] generator uses setup_node_env (#76422) (#76940)
a08e45e 
# Conflicts:
#	x-pack/plugins/security_solution/package.json
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-tavares paul-tavares paul-tavares left review comments

@kqualters-elastic kqualters-elastic kqualters-elastic approved these changes

@jonathan-buttner jonathan-buttner jonathan-buttner approved these changes

@marshallmain marshallmain marshallmain approved these changes

@michaelolo24 michaelolo24 michaelolo24 approved these changes

@XavierM XavierM Awaiting requested review from XavierM

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@oatkiller @kibanamachine @michaelolo24 @marshallmain @jonathan-buttner @kqualters-elastic @paul-tavares