Lint git index content on commit by cavokz · Pull Request #113300 · elastic/kibana

cavokz · 2021-09-28T17:28:05Z

This fixes two fundamental issues:

the list of files to be linted is built looking at the diffs of the ref specified with --ref (or otherwise the current index) with those on the filesystem
the content to be linted is read from the filesystem instead of the specified --ref or the one in the index

This fixes two fundamental issues: * the list of files to be linted is built looking at the diffs of the ref specified with `--ref` (or otherwise the current index) with those on the filesystem * the content to be linted is read from the filesystem instead of the specified `ref` or the one in the index

spalger

Thank you!! This is great

src/dev/file.ts

src/dev/precommit_hook/get_files_for_commit.js

… assignable to parameter of type 'never'"

Previously a refs interval was need needed, as leakage of the implementation of precommit_hook.

cavokz · 2021-09-29T09:27:45Z

I'm a bit perplexed by this (from commit_check_runner.sh):

!!!!!!!! ATTENTION !!!!!!!!
That check is intended to provide earlier CI feedback after we remove the automatic install for the local pre-commit hook.
If you want, you can still manually install the pre-commit hook locally by running 'node scripts/register_git_hook locally'
!!!!!!!!!!!!!!!!!!!!!!!!!!!

Is in the plans to actually drop the pre-commit hook usage on dev's machines?

To me it looks beneficial and actually useful to intercept as early as possible any gating issue to the PR merging, which is why I tried to improve the status quo.

test/scripts/checks/commit/commit_check_runner.sh

spalger · 2021-09-29T18:01:00Z

I'm a bit perplexed by this (from commit_check_runner.sh):
!!!!!!!! ATTENTION !!!!!!!!
That check is intended to provide earlier CI feedback after we remove the automatic install for the local pre-commit hook.
If you want, you can still manually install the pre-commit hook locally by running 'node scripts/register_git_hook locally'
!!!!!!!!!!!!!!!!!!!!!!!!!!!
Is in the plans to actually drop the pre-commit hook usage on dev's machines?

To me it looks beneficial and actually useful to intercept as early as possible any gating issue to the PR merging, which is why I tried to improve the status quo.

Yes, it's actually already done. The precommit hook isn't setup by default when you bootstrap. We found the precommit hook to mostly be annoying and that with good IDE integrations and auto-fix on save it seemed rather unnecessary. Though, like the message reads, users can opt into the precommit hook and have it installed. Additionally, we run the equivalent checks first on CI so that it can tell users as quickly as possible when they do have issues which would have been caught by the precommit hook (had they installed it).

spalger · 2021-09-29T18:10:21Z

src/dev/precommit_hook/get_files_for_commit.js

  const simpleGit = new SimpleGit(REPO_ROOT);
-  const gitRefForDiff = gitRef ? gitRef : '--cached';
-  const output = await fcb((cb) => simpleGit.diff(['--name-status', gitRefForDiff], cb));
+  const gitCatPrefix = gitRef ? gitRef + ':' : ':';


Do you think we could use a git command to resolve a git range to the latest sha so that we could pass things like --ref master^^...HEAD and then read the blobs from the sha pointed to by HEAD?

I can check. I also see the value of specifying a range.

With that use case in mind, I think that for the --ref usage I should simply revert to the original behavior. Which is, the range is used only to build the list of files. The linting is actually done on the files as-is on disk, effectively locking the right end of the range to HEAD.

Otherwise a given file would be linted as many time as the many commits that change it, possibly reporting problems that would otherwise elide in the whole range.

Hmmmmmm... I definitely think that we use the --ref more often to lint all the file that changed since a specific ref. I guess I just assumed that this would work that way too but maybe that was a bad assumption. Maybe we need to rename --ref or fix the --help text?

For instance, with the recent prettier update, we used it to help people --fix all the files that were modified in their PR

Do you think we could use a git command to resolve a git range to the latest sha so that we could pass things like --ref master^^...HEAD and then read the blobs from the sha pointed to by HEAD?

Forget my previous claims, I implemented this case ;)

cavokz · 2021-09-30T14:26:50Z

@elasticmachine merge upstream

kibanamachine · 2021-09-30T16:32:55Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #156751 failed 2f821dc
💛 Build #156334 was flaky a047174
💔 Build #156321 failed 32a86b7
💔 Build #156291 failed 5bc39ba
💔 Build #156174 failed ed59f48

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

spalger

LGTM, thanks for your help making this better!

This fixes two fundamental issues: * the list of files to be linted is built looking at the diffs of the ref specified with `--ref` (or otherwise the current index) with those on the filesystem * the content to be linted is read from the filesystem instead of the specified `ref` or the one in the index

kibanamachine · 2021-10-01T05:06:22Z

💚 Backport successful

Status	Branch	Result
✅	7.15
✅	7.x

The backport PRs will be merged automatically after passing CI.

This fixes two fundamental issues: * the list of files to be linted is built looking at the diffs of the ref specified with `--ref` (or otherwise the current index) with those on the filesystem * the content to be linted is read from the filesystem instead of the specified `ref` or the one in the index Co-authored-by: Domenico Andreoli <domenico.andreoli@elastic.co>

spalger · 2021-10-04T15:59:16Z

Sorry @cavokz, but it seems that scss linting is broken with this PR. I'm not sure what the issue is, but I'm going to need to revert this for now.

This reverts commit 92fe7f8.

This reverts commit 8422a24.

cavokz · 2021-10-04T16:03:22Z

Sorry @cavokz, but it seems that scss linting is broken with this PR. I'm not sure what the issue is, but I'm going to need to revert this for now.

I'd like to understand what got broken. I tested it with scss files. Do you have a link for me?

This reverts commit 4575dde.

spalger · 2021-10-04T16:10:00Z

I was able to reproduce the issue in #113570, which is admittedly a large reproduction case, but you can reproduce it by trying to run the pre-commit hook on any scss file with // comments like src/plugins/presentation_util/public/components/solution_toolbar/items/button.scss

cavokz · 2021-10-05T13:12:20Z

Sorry @cavokz, but it seems that scss linting is broken with this PR. I'm not sure what the issue is, but I'm going to need to revert this for now.

This was auto-backported to 7.x and 7.15, did you revert it from there as well?

spalger · 2021-10-12T20:52:26Z

Sorry @cavokz, but it seems that scss linting is broken with this PR. I'm not sure what the issue is, but I'm going to need to revert this for now.

This was auto-backported to 7.x and 7.15, did you revert it from there as well?

Sorry, missed this, but yes. Thanks for checking

cavokz requested a review from a team as a code owner September 28, 2021 17:28

cavokz added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v7.16.0 v8.0.0 labels Sep 28, 2021

spalger reviewed Sep 28, 2021

View reviewed changes

src/dev/file.ts Outdated Show resolved Hide resolved

src/dev/file.ts Outdated Show resolved Hide resolved

src/dev/file.ts Outdated Show resolved Hide resolved

spalger reviewed Sep 28, 2021

View reviewed changes

src/dev/precommit_hook/get_files_for_commit.js Outdated Show resolved Hide resolved

cavokz added 9 commits September 29, 2021 06:34

Drop spurious ':' in a rev:path spec argument

c13e6a8

Drop usedDeprecatedRules, not in the current eslint version

8710955

When --fix is specified, lint the files on FS

831c7c4

Rework the virtual file reader helper

ba15d67

Adjust file names in stylelint's output

5bc39ba

Give type to the report and fix "Argument of type 'LintResult' is not…

3abc9d0

… assignable to parameter of type 'never'"

Use more robust filename replacement logic

a7db6ad

Add workaround for relative paths when linting virtual files

32a86b7

precommit_hook --ref now truly handles a refspec

a047174

Previously a refs interval was need needed, as leakage of the implementation of precommit_hook.

cavokz enabled auto-merge (squash) September 29, 2021 14:20

cavokz requested a review from spalger September 29, 2021 14:33

spalger reviewed Sep 29, 2021

View reviewed changes

test/scripts/checks/commit/commit_check_runner.sh Outdated Show resolved Hide resolved

spalger reviewed Sep 29, 2021

View reviewed changes

Extend --ref to accept ranges again

2f821dc

cavokz disabled auto-merge September 30, 2021 13:47

Merge branch 'master' into eslint-run-on-commit-content

ab573a1

cavokz requested a review from spalger September 30, 2021 19:24

spalger approved these changes Sep 30, 2021

View reviewed changes

cavokz merged commit 92fe7f8 into elastic:master Oct 1, 2021

cavokz deleted the eslint-run-on-commit-content branch October 1, 2021 05:03

This was referenced Oct 1, 2021

[7.15] Lint git index content on commit (#113300) #113593

Merged

[7.x] Lint git index content on commit (#113300) #113594

Merged

spalger added a commit that referenced this pull request Oct 4, 2021

Revert "Lint git index content on commit (#113300)"

2b401d0

This reverts commit 92fe7f8.

spalger mentioned this pull request Oct 4, 2021

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

Merged

spalger added a commit that referenced this pull request Oct 4, 2021

Revert "Lint git index content on commit (#113300) (#113594)"

66d4fb4

This reverts commit 8422a24.

spalger added a commit that referenced this pull request Oct 4, 2021

Revert "Lint git index content on commit (#113300) (#113593)"

b2af24c

This reverts commit 4575dde.

spalger added the reverted label Oct 4, 2021

Conversation

cavokz commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cavokz commented Sep 29, 2021

Uh oh!

Uh oh!

spalger commented Sep 29, 2021

Uh oh!

spalger Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

cavokz Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

cavokz Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

spalger Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

spalger Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

cavokz Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

cavokz commented Sep 30, 2021

Uh oh!

kibanamachine commented Sep 30, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 1, 2021

💚 Backport successful

Uh oh!

spalger commented Oct 4, 2021

Uh oh!

cavokz commented Oct 4, 2021

Uh oh!

spalger commented Oct 4, 2021

Uh oh!

cavokz commented Oct 5, 2021

Uh oh!

spalger commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cavokz commented Sep 28, 2021 •

edited

Loading