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

undeprecate calling scripts from npm / yarn run #27850

Merged
merged 7 commits into from Jan 7, 2019

Conversation

Projects
None yet
3 participants
@mattkime
Copy link
Contributor

mattkime commented Dec 28, 2018

Summary

A number of (yarn|npm) run commands just told the user how to run the script. Its okay to run scripts from yarn without nagging.

Also added lint:fix, lint:es, lint:ts, lint:fix, lint:es:fix, lint:ts:fix commands which I hope are self explanatory.

Checklist

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

For maintainers

@mattkime mattkime requested a review from tylersmalley Dec 28, 2018

@mattkime mattkime added the :Dev Tools label Dec 28, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Dec 28, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 3, 2019

mattkime added some commits Jan 3, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 4, 2019

@mattkime

This comment has been minimized.

Copy link
Contributor

mattkime commented Jan 4, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 4, 2019

@@ -37,16 +37,16 @@
"preinstall": "node ./preinstall_check",
"kbn": "node scripts/kbn",
"es": "node scripts/es",
"elasticsearch": "echo 'use `yarn es snapshot -E path.data=../data/`'",
"elasticsearch": "node scripts/es snapshot -E path.data=../data/",

This comment has been minimized.

@tylersmalley

tylersmalley Jan 4, 2019

Member

Lets either remove this command, or leave it how it was. This was added to provide similar compatibility to what was prior to kbn-es, but it should not be how folks are running ES.

"lintroller": "echo 'use `node scripts/eslint --fix` and/or `node scripts/tslint --fix`' && false",
"makelogs": "echo 'use `node scripts/makelogs`' && false",
"mocha": "echo 'use `node scripts/mocha`' && false",
"lint": "node scripts/eslint && node scripts/tslint && node scripts/sasslint",

This comment has been minimized.

@tylersmalley

tylersmalley Jan 4, 2019

Member

Thoughts on instead calling the other yarn script from here?

yarn run lint:es && yarn run run lint:ts && yarn run run lint:sass

"lint:ts" : "node scripts/tslint",
"lint:sass" : "node scripts/sasslint",
"lint:fix": "node scripts/eslint --fix && node scripts/tslint --fix",
"lint:es:fix" : "node scripts/eslint --fix",

This comment has been minimized.

@tylersmalley

tylersmalley Jan 4, 2019

Member

I think we can remove the fix - you can call lint:es --fix

This comment has been minimized.

@mattkime

mattkime Jan 4, 2019

Contributor

@tylersmalley While thats true, its not documented. Is there another place where we could make this option obvious to devs?

This comment has been minimized.

@tylersmalley

tylersmalley Jan 4, 2019

Member

It's in the help text:

Fixing problems:
  --fix                          Automatically fix problems
  --fix-dry-run                  Automatically fix problems without saving the changes to the file system

Additionally, if it fails it calls out to run with --fix

node scripts/eslint.js

/mnt/c/elastic/kibana/src/cli/serve/read_keystore.js
  26:29  error  Unexpected space before function parentheses  space-before-function-paren

/mnt/c/elastic/kibana/x-pack/plugins/apm/index.js
  20:15  error  Replace `'kibana',·'elasticsearch',·'xpack_main',·'apm_oss',·'upgrade_assistant'` with `⏎······'kibana',⏎······'elasticsearch',⏎······'xpack_main',⏎······'apm_oss',⏎······'upgrade_assistant'⏎····`  prettier/prettier

✖ 2 problems (2 errors, 0 warnings)
  2 errors and 0 warnings potentially fixable with the `--fix` option.

Adding mappings to each command line argument can't scale.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 4, 2019

@mattkime

This comment has been minimized.

Copy link
Contributor

mattkime commented Jan 4, 2019

retest

"lintroller": "echo 'use `node scripts/eslint --fix` and/or `node scripts/tslint --fix`' && false",
"makelogs": "echo 'use `node scripts/makelogs`' && false",
"mocha": "echo 'use `node scripts/mocha`' && false",
"lint": "npm-run-all lint:es lint:ts lint:sass",

This comment has been minimized.

@tylersmalley

tylersmalley Jan 4, 2019

Member

I don't think this is worth adding a dependency and it's 9 dependencies for

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 5, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 5, 2019

@mattkime

This comment has been minimized.

Copy link
Contributor

mattkime commented Jan 5, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 5, 2019

@mattkime

This comment has been minimized.

Copy link
Contributor

mattkime commented Jan 5, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 5, 2019

@mattkime mattkime merged commit 9311f19 into elastic:master Jan 7, 2019

1 of 2 checks passed

kibana-ci Build finished.
Details
CLA Commit author is a member of Elasticsearch
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment