Teach CircleCI to run eslint for files modified in the topic branch #6625
Conversation
Modify a file in this branch to break the rules. This is the only file we would like eslint to report as an error. Commited with --no-verify as pre-commit hook wouldn't let you otherwise.
otherwise, just exit 0 - as we are about to use it to lint branch files in CircleCI.
origin/master is the reference we want to compare the topic branch with, as master in CircleCI setup could be (and it is) pointing to a different commit. CircleCI seems to do a shallow clone of the topic branch, which means you can no longer rely on having the whole history of the repo and also that some aliases (such as master) could point to places you didn't expect.
ghost
mentioned this pull request
As it was a commit to test whether the feature was working.
As this was a commit to test whether the feature was working.
nb
added
[Status] Needs Review
| parallel: true | ||
| override: | ||
| - bin/run-lint : | ||
| parallel: true |
There was a problem hiding this comment.
Are we going to lose the checks running in parallel?
There was a problem hiding this comment.
The files modifier accepts only globs, so we cannot use them to write git expressions. We could find a way to set parallelism up again by using env variables in run-lint script, though.
The reason I was OK switching off parallelism and not pursuing the second approach was: if we are going to run eslint only for the files modified in the branch, I guess they are a handful, not a lot. Parallelism, then, is not going to accelerate things that much and, in return, we have a clearer and more understandable solution (without that logic in the script). My assumptions about the size and number of files by PR may be totally wrong as I am still not very familiar with the common practices around here - and so the gains of parallelization in this specific case could be bigger than I thought. What do you think?
There was a problem hiding this comment.
I think that's fine, though how does this work for the master merges?
There was a problem hiding this comment.
The run-lint script is not going to run eslint.
As the HEAD in the master branch is pointing to the same place than origin/master, the result of the git expression would be void. Ergo, the run-lint script would not run.
But your question makes me wonder: I am assuming calypso is using CircleCI to do quality-checks on branches/PR before they are integrated into master. Is it useful outside that? I mean, do you expect CircleCI to also run on master?
cc @blowery
There was a problem hiding this comment.
Yes, it’s important to run on master, too – you never know what will happen after the merge. Also, need to make sure we run in on direct commits to master (I think we have that disabled, but still, better safe than sorry).
I'm not sure about this part. It will put the burden on PR authors to ensure that any files they modify are entirely compliant with the latest Calypso lint config, which will mean rewriting a lot of irrelevant code. |
|
@nylen the main idea is that the next step of the project is relegating the errors on lines yo didn’t modify to warnings. |
|
That would be preferable, but seems beyond the capabilities of the standard version of |
|
@nylen definitely, @nssw started working on it in another repo, we might bring it in in Calypso: https://github.com/nssw/eslines |
|
I was about to mention that :) The independent parts (differ to take lines from git-diff + downgrading ESLint errors to warnings) are mostly working but I need to work on putting them together and some minor things. As @nb noted the place for this is probably calypso repo, so I need also figure out a way to integrate it while preserving testing, etc. cc @nylen |
ghost
closed this
nb
removed
the
[Status] Needs Review
4 participants
This PR seeks to teach CircleCI to run-lint for the files modified in the topic branch. As a consequence, if we set to
errorseslint rules which are nowwarnings, CircleCI report would only give contextual information about the PR, not the whole errors in the codebase.Test case
You might want to see how this works in the test case PR #6658 and what the results would be in Circle CI here. The test case sets to
errorthe rulespace-before-blocksand introduces a break in the rule in fileclient/components/accordion/section.jsx. Expected result is that only that file is linted when running CircleCI although there are more cases in the codebase where that rule is not followed.cc @nb