Skip to content

kubevirt: add lint scripts #1826

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
Merged
openshift-merge-robot merged 2 commits into from
Jul 15, 2019
Merged

kubevirt: add lint scripts #1826

openshift-merge-robot merged 2 commits into from
Jul 15, 2019

Conversation

@atiratree
Copy link
Member

@atiratree atiratree commented Jun 26, 2019 •
edited
Loading

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 26, 2019
@spadgett spadgett added this to the v4.2 milestone Jun 26, 2019
mareklibra
mareklibra reviewed Jun 27, 2019
View reviewed changes
frontend/packages/kubevirt-plugin/package.json Outdated
Copy link
Contributor

@mareklibra mareklibra Jun 27, 2019

Choose a reason for hiding this comment

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

I prefer lint:fix.
However recent scripts use lint-fix convention so we should stay aligned.

Copy link
Member Author

@atiratree atiratree Jun 27, 2019

Choose a reason for hiding this comment

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

I decided to leave only lint and one can use lint --fix like in the main package.json

frontend/packages/kubevirt-plugin/package.json Outdated
Copy link
Contributor

@mareklibra mareklibra Jun 27, 2019

Choose a reason for hiding this comment

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

It would be great if we can reuse the script from frontend/package.json via sort of yarn -cwd ../.. lint [TARGET].
Probably a new rule without . directory would need to be defined in the top-level package.json.

Copy link
Member Author

@atiratree atiratree Jun 27, 2019

Choose a reason for hiding this comment

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

good idea, created a #1813 for that so it is possible to lint even from top level

@mareklibra
Copy link
Contributor

mareklibra commented Jun 27, 2019

Great improvement for development on just kubevirt-plugin. The linter takes approx. 3 seconds instead of former 70 secs on my machine.

@atiratree atiratree mentioned this pull request Jun 27, 2019
Merged
@atiratree atiratree force-pushed the kubevirt.addScripts branch from af830f0 to 9324140 Compare June 27, 2019 13:56
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 27, 2019
@atiratree atiratree force-pushed the kubevirt.addScripts branch from 9324140 to 00d2149 Compare June 28, 2019 13:42
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 28, 2019
@atiratree
Copy link
Member Author

atiratree commented Jun 28, 2019

/retest

1 similar comment
@atiratree
Copy link
Member Author

atiratree commented Jun 28, 2019

/retest

vojtechszocs
vojtechszocs suggested changes Jul 2, 2019
View reviewed changes
frontend/packages/kubevirt-plugin/package.json Outdated
Copy link
Contributor

@vojtechszocs vojtechszocs Jun 26, 2019

Choose a reason for hiding this comment

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

Nit: please put scripts before dependencies to follow existing convention.

Copy link
Member Author

@atiratree atiratree Jul 3, 2019

Choose a reason for hiding this comment

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

fixed

@vojtechszocs
Copy link
Contributor

vojtechszocs commented Jul 2, 2019

LGTM with minor comment.

@vojtechszocs
Copy link
Contributor

vojtechszocs commented Jul 2, 2019 •
edited
Loading

Great improvement for development on just kubevirt-plugin. The linter takes approx. 3 seconds instead of former 70 secs on my machine.

@mareklibra On my machine, it takes even more:

$ time yarn lint
yarn run v1.15.2
$ eslint --ext .js,.jsx,.ts,.tsx --color .
...
stuff
...
Done in 80.39s.

real	1m20.884s
user	2m4.998s
sys	0m4.577s

The problem appears to be TypeScript parser for ESLint being slow, which is something we can't impact directly (unless we upgrade to a newer version that has improved performance).

@atiratree atiratree force-pushed the kubevirt.addScripts branch from 00d2149 to 67aadcb Compare July 3, 2019 09:25
vojtechszocs
vojtechszocs suggested changes Jul 12, 2019
View reviewed changes
@atiratree atiratree force-pushed the kubevirt.addScripts branch from 67aadcb to f817afd Compare July 15, 2019 08:50
@atiratree atiratree force-pushed the kubevirt.addScripts branch from f817afd to 7f0981b Compare July 15, 2019 08:53
@atiratree
Copy link
Member Author

atiratree commented Jul 15, 2019

/test e2e-aws-console-olm
/test e2e-aws-console
/test e2e-aws

@vojtechszocs
Copy link
Contributor

vojtechszocs commented Jul 15, 2019

/assign

@vojtechszocs
Copy link
Contributor

vojtechszocs commented Jul 15, 2019

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: suomiy, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2019
@openshift-merge-robot openshift-merge-robot merged commit b6b32b3 into openshift:master Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jelkosz jelkosz Awaiting requested review from jelkosz

2 more reviewers

@mareklibra mareklibra mareklibra left review comments

@vojtechszocs vojtechszocs vojtechszocs requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vojtechszocs vojtechszocs

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

v4.2

Development

Successfully merging this pull request may close these issues.

7 participants

@atiratree @mareklibra @vojtechszocs @openshift-ci-robot @spadgett @openshift-merge-robot @suomiy