Reconcile gitlab lint jobs and commit-code-check.sh

Instead of writing shell directly in .gitlab-ci.yml, we could have bash script(s) in the repo that can be run both locally and invoked from .gitlab-ci.yml.

Yeah splitting core checks into multiple files that can be invoked separately

This is one step towards that: 📌 Simplify commit-code-check.sh: reduce repetition Needs work .

And more importantly: ✨ Make it possible for contrib modules to reuse core's commit-code-check.sh Needs work — this would help contrib a lot, as well as updating the d.o GitLab CI Template for contrib modules!

What about just keeping commit-code-check but passing in a parameter to select the task to run, cspell, phpstan, eslint, stylelint, ckeditor5, js.

There are some gitlab-specific things in some of the gitlab lint jobs now, like phpstan producing code quality report output, but we could probably add a check for a gitlab variable in the script, and determine the output based on that too. Or add an explicit --gitlab option for that switch and use that in the pipelines.

I am never that comfortable with bash scripting, so this might be a bad idea, but what about:

1. Create an individual script in core/scripts/dev for each linting step, which gitlab can then call. All of the code is still in the core repo so it should be just as, or more, maintainable that way.
2. commit-code-check.sh then calls out to each of these scripts so there's still an easy single command for linting things locally.

As I think about this I don't mind how this is accomplished, as long as we avoid duplicating all the file selection work that commit-code-check does at the beginning.

I made an attempt at breaking commit-code-check.sh into task specific tasks. The linting is failing because I forced some errors so error output could be checked both locally and on gitlab.

Not sure why https://git.drupalcode.org/project/drupal/-/jobs/376422 is failing

The phpcs and cspell makes sense.

@smustgrave see @quietone's comment directly above yours:

The linting is failing because I forced some errors so error output could be checked both locally and on gitlab.

Back to needs review for that - we need to review that the failing gitlab jobs fail in the correct way.

Correct my comment says I see the 2 failures for phpcs and cspell

But the one I linked doesn't make sense why it's failing.

That's the same as the phpstan job in HEAD, the linting issues show up in the code quality report (in a tab) on the pipeline, and inline in the MR.

Just found 📌 Incorporate improvements to how contrib runs PHPStan to core RTBC which will address my concern. Could the changes to generate failures be reverted then can RTBC.

Could the changes to generate failures be reverted then can RTBC.

Sure those errors can be removed but I think they should stay until there are code reviews here and agreement on the approach.

Also, I am just learning gitlabci so there may be improvements that should be made in this issue.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Retook a look and don't see any issue with breaking up the scripts.

May need a change record though, imagine someone out there may be leveraging those scripts.

There's a lot to unpack in this merge request but wanted to point related to these changes the current spellcheck logic isn't great and probably not behaving as intended. The way it checks the remote branch actually checks against the issue fork which can relies on both the branch existing and actually matching the branch we're merging against. This only kinda works on recent forks but on _old_ forks it can be completely broken and fail entirely.

This probably needs its own issue unless this is close to being resolved.

This does not need a change record.

MR has some failures.

Coming here from 📌 Allow commit-code-check.sh to skip non-PHP checks with new opt-in flag Closed: duplicate where I was looking to make this tool more flexible, e.g. not failing on steps that require yarn/node runtime available locally. By reasons of personal preference I don't have these dependencies installed locally but will still benefit from certain checks like phpstan and phpcs, and the --cached flag to limit the linting to changed files.

I'm going to take a stab at helping this along. One immediate observation is that we could make this less complex by simply looking for CI=1, which GitLab CI sets and is a pretty typical pattern across CI tools to hint to tooling that you are running in a CI environment. I think this could reduce the amount of code required in the individual checks to switch based on its runtime environment.

@bradjones1, I think that enhancements to make this run without yarn/node runtime available should be in a separate issue. As @neclimdul pointed out there is already a lot in this MR to unpack.

@quietone as I'm working on refactoring this I think the way forward for both this MR and my angle can be handled in one pass. As we need to be able to call individual check(s) from the CI jobs that makes it possible to call individual check(s) locally, too, which suits my needs.

@bradjones1, I respectfully disagree. This is to reconcile two 'things', not to add features.

I've pushed a rebase to bring this inline with changes in .gitlab-ci.yml et. al. since this was last touched.

Re: #27, I hope the rebase allays your concerns. The thrust of the MR already was to delegate job calls to the code in core/scripts/dev, so there's only one place to find the code for both CI and local invocations.

What I've done here is to simply refine that to not call a specific script directly but specify a check to run with the new --check flag. The side effect, as it were, of this is that you can do the same locally. This also results in a lot less boilerplate code having to live in each of the check files to call setup functions. I get where you're going with your objection but this is what I meant by getting this "feature" in the course of achieving the main goal of this issue.

This will need a bit more refinement as some of the jobs have been changed in the meantime (e.g., the "compile" check is now rolled into "CSS").

Fixed some other items such as a wrong exit value on failure of an individual job when calling the script with no --check option. Importantly this does not change the out-of-the-box behavior calling this locally.

Also using the predefined GitLab CI variable $CI to detect a CI environment.

This is still NW as the validatable config job in particular needs abstracted into a bash script, and we need to validate these still match what's in .gitlab-ci.yml currently across the board.

Thanks for updating the MR! I looked at it and it does not change my mind about scope creep. The scope guidelines → begin with "Drupal core issues should include only a single scope, for example fixing a single bug with test coverage, or making one specific type of cleanup." I think that is applicable here.

Also, the MR is adding a new parameter. That was suggested in #7 and disagreed with in #8 so it was not pursued. It also introduces a behavior change because the parameter is required. I agree that these are useful. But, like the existing issue to have commit-code-check work for contrib it should be in separate issue so this remains solely about parity between the gitlab jobs and the script.

I respectfully disagree with the take that this is scope creep.

The MR that needed rebasing introduced a group of entirely new scripts, which I could make an argument is a "new feature" vs. having the existing script be able to run single checks on demand. I am making the case that the MR as it sits now is actually less complex than calling these new scripts directly.

The new argument is also mischaracterized as required. It is not. It's only "required" if you want to run a single check instead of the full suite. The script runs pretty much identically to the way it did before when called with no arguments.

There is going to be something "new" here regardless, to accomplish the reconciliation of commit-code-check.sh to GitLab CI job scripts. Put another way, if the initial solution to this task looked like this and I hadn't come in and mentioned that it _also_ happens to make the tool more useful in other circumstances, I feel like this wouldn't even be a discussion.

I feel like my changes are being reviewed in a way that's skewed toward rejection because of how I came to this issue, rather than evaluating the solution on its own merits. Happy side-effects aren't scope creep IMO.