Run same checks as committers do on DrupalCI

This looks interesting. Not sure if you are ready for questions/review yet, as it is clear WIP. But the first thing I noticed is that it appears that the phpcs section in the new script does not write anything in the dispatcher log. Is that intentional? At least, I could not find 'phpcs' in the log, nor was there any output in the artifacts.

Please excuse me, and feel free not to answer in depth (or at all) if this is all still to be done.

Yes thanks, I did see that output. But when there is nothing produced (because phpcs passes everything) I could not see any evidence that is has actually been run. I was just thinking about some time in the future, when some bug stops the phpcs section from being triggered, it could be a while before it is noticed, if we get used to seeing no output and no record that it has been run (when all is passing). But this is a minor point, I did not want to distract you from the bigger tasks.

@jonathan1055 Making "Custom commands failed" actually raise a fail and mark the issue NW would be a Drupal infrastructure followup to this issue, I think. That said, committers would still not commit the patch with the "Custom commands failed" test result, and it's still a step up from where we are currently (where the error itself is only discovered when the committer goes to commit the patch on their local CLI).

The one thing I would suggest is that we dont keep this in core/scripts/ and actually start another scripts folder at the root of the project for development purposes.

Given that this script isnt intended to be part of the product, it also cordon's it off into an area that we can change, at will, without worry of whether or not somebody has come to rely on it.

Couple things in the patch seem like they might have been specific to running this in the actual commit workflow vs. on DrupalCI; questions below.

1. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,313 @@
   +# cSpell:disable
   ...
   +# Set up variables to make coloured output simple.
   ...
   +  # For DrupalCI / default behaviour this is the list of all changes in the
   

   While it's fine to disable cspell on the file, we probably should still use core's standard AE spellings here?

2. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,313 @@
   +  # Check staged files only.
   +  if git rev-parse --verify HEAD >/dev/null 2>&1
   

   Is this relevant for DrupalCI? (Or are the other cases?)

   Presumably the fail-patches prove that it works. It might be good to have a fail-patch that has a fail for each kind of check, just to make sure they're all running consistently. I read through https://www.drupal.org/pift-ci-job/1863278 and it seems to hit most of them, but not a PHPCS fail? Edit: It's there; just missed it searching the output for "PHPCS" because obviously the new yaml file is also being checked and passes.

3. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,313 @@
   +# This script assumes that composer install and yarn install have already been
   +# run and all dependencies are updated.
   

   Can we guarantee this will always be the case on DrupalCI? I didn't realize yarn install would happen at all. What would a fail look like if they hadn't been?

Also, should we add documentation about how to run the thing locally?

+++ b/core/lib/Drupal.php
@@ -142,7 +144,7 @@ class Drupal {
-   * @var \Symfony\Component\DependencyInjection\ContainerInterface|null
+   * @var

So this was intended to be the PHPCS fail, but it didn't actually cause PHPCS to fail. Did PHPCS actually run? Or is this rule not enabled?

Never mind; I just didn't read far enough down in the result because of newlines. It is there.

Come to think of it, maybe it's worth retaining all the different logic for different git statuses so that it can be used locally (if we document it better).

Also a CR.

Couple other thoughts:

1. Do we expect the 80% usecase to be DrupalCI results, or results from running it locally? I'd expect the former. The color codes are nice on the CLI but pollute the DrupalCI result with what are, to the uninitiated, random characters.
2. Similarly, for the DrupalCI case, could we omit outputting results for passing files? For a large patch, it's going to be hard to find the one coding standards error in the output for dozens of changed files. I don't know if the individual tools support this or not but it would help people understand the results (and avoid my mistake above).

Re: #18 I don't think a help flag in the script counts as documentation; the problem is that no one is going to find this to use it locally. I think the solution might actually be handbook documentation; maybe under https://www.drupal.org/docs/develop/standards or a level up? Then the CR could include a brief summary and link that handbook doc.

Bumping priority since this is a blocker for using merge requests for core. I debated on critical but there are technically two workarounds (simplest probably being committers downloading the plain diff and committing it that way, honestly).

+++ b/core/scripts/dev/commit-code-check.sh
@@ -68,6 +69,19 @@
+# Set up variables to make colored output simple. Color output is disable on

Typo: disabled.

Re: #28 That is only broken for right now for Merge Requests.

Everything in #27 worked as expected.

Two minor grammar problems from the interdiff. It is worth getting these comments right.

+# If the FILES is empty the assume we're on a branch and we want to diff
+# against the Drupal branch. This always DrupalCI to do MR testing.

1. the assume -> then assume
2. "This always DrupalCI to do MR testing." is not a complete sentence. Is there a word missing?

I think this is probably good to go, if we can get that handbook page somewhere?

perhaps the CR should just point there so we only have one place to maintain.

+1.

Let's get this up and running - it's getting urgent with the number of merge requests against core.

Mostly nitpicks that could be fixed on commit but thought it might be worthwhile fixing the file permission messages in a patch.

1. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,365 @@
   +# - PHPCS checks php and yaml files.
   

   Nit: s/php/PHP, s/yaml/YAML

2. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,365 @@
   +# - Eslint checks javascript files.
   

   Nit: s/javascript/JavaScript

3. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,365 @@
   +# - Stylelint checks css files.
   

   Nit: s/css/CSS

4. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,365 @@
   +        printf "${red}git pre-commit check failed:${reset} file $FILE should be 644 not $STAT\n"
   ...
   +    printf "${red}git pre-commit check failed:${reset} file in vendor directory being committed ($FILE)\n"
   ...
   +    printf "${red}git pre-commit check failed:${reset} file in core/node_modules directory being committed ($FILE)\n"
   

   Should we change the message to not mention git pre-commit check given that this is now run as part of the CI process?

5. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -0,0 +1,365 @@
   +    # Work out the root name of the Javascript so we can ensure that the ES6
   

   Nit: s/Javascript/JavaScript

Committed 5486534 and pushed to 9.2.x. Thanks!

Leaving open for backport to 9.1.x.

Adding credit for @Mixologic as well; while he didn't comment on the issue much he did help fix the approach for MRs.

Also made a couple improvements to the CR, including fixing a11y for the link (always use a meaningful link text, folks) as well as explaining what actually happens when the checks fail and how to find out what the failures are.

Oh and also +1 for the backport to at least 9.1.x. I'll ask @catch for thoughts on 8.9.x.

So 9.2.x is the only branch where mistakes will accumulate (all previous branches are dead-ends), BUT 9.1.x is the canonical branch for docs now, so we should definitely backport it there (as well as the potential for tests to not run on 9.2.x at all). For 8.9.x it seems more likely we'll have diverging patches that need their own test run, and also people will probably continue to look at 8.9.x docs when working on 8.9.x sites in case the 9.x docs have changed. So... let's backport it to both IMO. If there is an unforseen problem, we can roll it back or deal with it when it comes up.

Cherry-picked to 9.1.x. Based on #44 we need new patch for 8.9.x so moving back to needs work for that.

Rolling 8.9.x patch

Not sure what the state of the MR is, so adding this a POP (=Plain Ol' Patch) based on @laurii's commit

Let's try this then!
#IHateTestBot

3rd time lucky!
#TestBotMightBeOK

Who needs 3 attempts when you can use 4! 🤦

Seems to work and doing no spell checks.

Relevant console log for the 8.9.x patch:

00:01:35.371 ----------------   Starting container_command.commit-checks   ----------------
00:01:35.379 Directory created at /var/lib/drupalci/workspace/jenkins-drupal_patches-71626/ancillary/container_command.commit-checks
00:01:35.379 Container command.
00:01:35.379 core/scripts/dev/commit-code-check.sh --drupalci
00:01:35.946 
00:01:35.946 ----------------------------------------------------------------------------------------------------
00:01:35.946 Checking core/scripts/dev/commit-code-check.sh
00:01:35.946 
00:01:35.946 core/scripts/dev/commit-code-check.sh passed
00:01:35.946 
00:01:35.947 ----------------------------------------------------------------------------------------------------
00:01:35.947 Checking core/drupalci.yml
00:01:35.947 
00:01:36.105 PHPCS: core/drupalci.yml passed
00:01:36.105 core/drupalci.yml passed
00:01:36.105 
00:01:36.105 ----------------------------------------------------------------------------------------------------
00:01:36.120 ---------------- Finished container_command.commit-checks in 0.748 seconds ---------------- 

Compare to relevant console log from 9.2.x patch:

00:02:57.131 ----------------   Starting container_command.commit-checks   ----------------
00:02:57.138 Directory created at /var/lib/drupalci/workspace/jenkins-drupal_patches-71431/ancillary/container_command.commit-checks
00:02:57.138 Container command.
00:02:57.138 core/scripts/dev/commit-code-check.sh --drupalci
00:02:59.669 CSpell: Files checked: 2, Issues found: 0 in 0 files
00:02:59.689 
00:02:59.689 CSpell: passed
00:02:59.689 
00:02:59.689 ----------------------------------------------------------------------------------------------------
00:02:59.690 Checking core/scripts/dev/commit-code-check.sh
00:02:59.690 
00:02:59.690 core/scripts/dev/commit-code-check.sh passed
00:02:59.690 
00:02:59.690 ----------------------------------------------------------------------------------------------------
00:02:59.690 Checking core/drupalci.yml
00:02:59.690 
00:02:59.855 PHPCS: core/drupalci.yml passed
00:02:59.855 core/drupalci.yml passed
00:02:59.855 
00:02:59.855 ----------------------------------------------------------------------------------------------------
00:02:59.871 ---------------- Finished container_command.commit-checks in 2.739 seconds ---------------- 

Committed/pushed the backport to 8.9.x, thanks! Let's see how this goes, can always roll back if there's a problem.