Skip to content

chore: add modifications to staging automatically after eslint fix#113443

Merged
suchcodemuchwow merged 4 commits intoelastic:masterfrom
suchcodemuchwow:precommit-stage-lint-fixes
Oct 1, 2021
Merged

chore: add modifications to staging automatically after eslint fix#113443
suchcodemuchwow merged 4 commits intoelastic:masterfrom
suchcodemuchwow:precommit-stage-lint-fixes

Conversation

@suchcodemuchwow
Copy link
Copy Markdown
Contributor

Closes #52722

After precommit hook runs with --fix flag changes are not added to staging. However it also does not
validate staging area since eslint is only looking for last changes on file not staging area this
results fellows to commit with linting errors which fails in CI. This commit resolves this issue by
adding fixed files right after linting to staging area.

@suchcodemuchwow suchcodemuchwow self-assigned this Sep 29, 2021
@suchcodemuchwow suchcodemuchwow added the Team:Operations Kibana-Operations Team label Sep 29, 2021
@mistic mistic added auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0 labels Sep 29, 2021
After precommit hook runs with --fix flag changes are not added to staging. However it also does not
validate staging area since eslint is only looking for last changes on file not staging area this
results fellows to commit with linting errors which fails in CI. This commit resolves this issue by
adding fixed files right after linting to staging area.

Closes #52722
@suchcodemuchwow suchcodemuchwow marked this pull request as ready for review September 30, 2021 12:47
@suchcodemuchwow suchcodemuchwow requested a review from a team as a code owner September 30, 2021 12:47
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@mistic
Copy link
Copy Markdown
Contributor

mistic commented Sep 30, 2021

@elasticmachine merge upstream

@suchcodemuchwow
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @suchcodemuchwow

@suchcodemuchwow suchcodemuchwow removed the request for review from mistic October 1, 2021 11:51
@suchcodemuchwow suchcodemuchwow merged commit 5df9d41 into elastic:master Oct 1, 2021
@suchcodemuchwow suchcodemuchwow deleted the precommit-stage-lint-fixes branch October 1, 2021 11:53
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 1, 2021
…lastic#113443)

After precommit hook runs with --fix flag changes are not added to staging. However it also does not
validate staging area since eslint is only looking for last changes on file not staging area this
results fellows to commit with linting errors which fails in CI. This commit resolves this issue by
adding fixed files right after linting to staging area.

Closes elastic#52722

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 1, 2021
…113443) (#113611)

After precommit hook runs with --fix flag changes are not added to staging. However it also does not
validate staging area since eslint is only looking for last changes on file not staging area this
results fellows to commit with linting errors which fails in CI. This commit resolves this issue by
adding fixed files right after linting to staging area.

Closes #52722

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Baturalp Gurdin <9674241+suchcodemuchwow@users.noreply.github.com>
tylersmalley added a commit that referenced this pull request Oct 1, 2021
tylersmalley added a commit that referenced this pull request Oct 1, 2021
@tylersmalley
Copy link
Copy Markdown
Member

tylersmalley commented Oct 1, 2021

Sorry @suchcodemuchwow, had to revert this change. It resulted in rather unexpected failures for other PRs with errors like:

09:01:12   succ [eslint] 16 files linted successfully
09:01:12  ERROR 
09:01:12        x-pack/plugins/enterprise_search/public/applications/app_search/components/curations/views/curation_suggestion/curation_result_panel.scss
09:01:12         9:62  ✖  Unknown word   CssSyntaxError
09:01:12  
09:01:12  ERROR [stylelint] errors

The error seems similar to what happens if you directly pass the SCSS to eslint node scripts/eslint.js src/plugins/discover/public/application/components/doc_viewer/doc_viewer.scss. If the PR that failed didn't have a change to this file, is it expected that it would have even been covered by the Quick Commit Check/precommit hook?

@suchcodemuchwow
Copy link
Copy Markdown
Contributor Author

Hey @tylersmalley, I'm not quite sure how it happened and if linting error happens it should have yelled & cancel the commit even before committing locally. The Linting process/PR change works in this way:

  1. It first looks at files git diff --cached or git diff [ref]
  2. Based on the file types it runs relevant linter. Eslint for js & ts, Stylelint for sass, scss.
  3. If --fix option has been passed it fixes the fixable errors and add those files to staging (I believe this is the part where it is messing somehow, if it's relevant to this PR). However if the errors are not fixable by linter it should fail to commit and yell.

Note: node scripts/eslint.js src/plugins/discover/public/application/components/doc_viewer/doc_viewer.scss AFAIK eslint should not run against scss based on the code flow.

spalger added a commit that referenced this pull request Oct 4, 2021
@spalger spalger removed the reverted label Oct 4, 2021
@spalger
Copy link
Copy Markdown
Contributor

spalger commented Oct 4, 2021

Unreverted, looks like issue was actually in #113300

spalger added a commit that referenced this pull request Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes Team:Operations Kibana-Operations Team v7.16.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Precommit hook should take into account staged changes

7 participants