Problem/Motivation

Now that Gitlab is running for core its time to clean up how codequality is reported. Currently all code quality steps like esling, phpcs, phpstan, stylelint all al reported as "tests". Which is technically true, but its also possible to output those reports and have them reported in a separate tab. This would also allow showing the problems inline in the merge requests.

See: https://docs.gitlab.com/ee/ci/testing/code_quality.html#implement-a-cust...

Known issues.
We have Gitlab ultimate, so we COULD get inline errors in the mergerequests.

Unfortunately, it is only displayed on the child pipelines, this means no inline help on the MR... :disappointed: I was secretly already dreading if the child pipeline setup was going to make us run into some issues.
There is an open issue (for years) around this. But that doesnt seem to be close. We could have inline error messages in the pipeline, but not if we use childs. (https://gitlab.com/gitlab-org/gitlab/-/issues/215725).

Possible tools per lint step
The tools we use and how they could integrate into gitlab:

eslint

Add the flag --format gitlab

PHP Code Sniffer

No real support, but seems easy enough:
https://github.com/micheh/phpcs-gitlab or https://daniel-r-ness.medium.com/converting-phpcs-output-to-gitlab-codec...

PHPStan

phpstan analyze --error-format=gitlab

stylelint

https://github.com/leon0399/stylelint-formatter-gitlab

Dependency evaluation; all these are dev dependencies that do a single small task of converting tool output to a format GitLab can read, none of this is needed at runtime and will in effect only be executed on GitLab.

micheh/phpcs-gitlab

Release cycle: two releases in 2020, none since; both the plugin and the GitLab output format are stable and seemingly require no maintenance.

Security policy: none

Maintainer: Michel Hunziker https://github.com/micheh

eslint-formatter-gitlab

Release cycle: major releases approximately once a year, follows strict semver and updates when NodeJS is updated.

Security policy: none

Maintainer: Remco Haszing https://gitlab.com/remcohaszing

stylelint-formatter-gitlab

Release cycle: three releases in four years

Security policy: none

Maintainer: Leonid Meleshin https://gitlab.com/leon0399

Steps to reproduce

Run a pipeline, in the childpipeline there is a new Codequality tab. See draft MR for an mr with these changes and some real errors.

Proposed resolution

Add a few packages for the reports and export everything as codequality artifacts.

Remaining tasks

  1. Implement PHPStan report
  2. Implement PHPCS report
  3. Implement eslint report
  4. Implement stylelint report
  5. Decide if the dependencies are ok like this
  6. Follow up to investigate issue where child pipelines do not report codequality to their parents. See: https://gitlab.com/gitlab-org/gitlab/-/issues/215725

User interface changes

Tests tab in gitlab now only shows tests, new tab codequality with codequality issues.

Code quality tab on pipelines showing various code style errors in a summary

Code quality tab on pipelines showing various code style errors in a summary

Code quality tab on pipelines showing various code style errors in a summary

API changes

Data model changes

Release notes snippet

Three new development dependencies provide GitLab-compatible output for PHP CodeSniffer, ESLint and Stylelint.

CommentFileSizeAuthor
#13 Screenshot_20230916_182222_Chrome.jpg832.14 KBbbrala
#12 Screenshot from 2023-09-16 16-53-26.png173.69 KBcatch
#12 Screenshot from 2023-09-16 16-52-12.png102.62 KBcatch

Issue fork drupal-3387339

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bbrala created an issue. See original summary.

bbrala opened merge request !4786

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I also found stylelint-formatter-gitlab over in #3386481: Add stylelint-junit-formatter to package.json, happy to merge that issue with this one.

I think we should add the tools to composer.json/package.json as appropriate instead of adding them at build time.

cmlara’s picture

cmlara
he/him
commented
Related issues: +#3371868: Generate code quality reports

Linking the related feature request for this to be enabled in Contrib templates.

If these were to be installed as part of cores setup it would likely make contrib testing easier too.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs work

bbrala opened merge request !4793

bbrala’s picture

bbrala
Location Netherlands
commented
Issue summary: View changes
bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review
bbrala’s picture

bbrala
Location Netherlands
commented
Title: Integrate codequality reports into Gitlab » [PP-1] Integrate codequality reports into Gitlab
Parent issue: » #3387107: [meta] GitLab CI feature parity with DrupalCI
Related issues: +#3387107: [meta] GitLab CI feature parity with DrupalCI, +#3387503: Move Gitlab linting steps to main job

Postponed on #3387503: Move Gitlab linting steps to main job

catch’s picture

catch
he/him
Primary language English
commented
Title: [PP-1] Integrate codequality reports into Gitlab » Integrate codequality reports into Gitlab
bbrala’s picture

bbrala
Location Netherlands
commented

Ready for review and such again

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
StatusFileSize
new Screenshot from 2023-09-16 16-52-12.png102.62 KB
new Screenshot from 2023-09-16 16-53-26.png173.69 KB

No substantive review but adding screenshots, what a difference from 'build successful' or 'custom commands failed'.

bbrala’s picture

bbrala
Location Netherlands
commented
Issue summary: View changes
StatusFileSize
new Screenshot_20230916_182222_Chrome.jpg832.14 KB

Updated is.

Alsl added screenshot of the mr overview page, that already shows the cs issues also.

dww’s picture

dww
we/he/they
commented

This is fantastic, thanks! Hope to review the code ASAP.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think we should add the dependencies to composer.json and package.json.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

There are some other bits we can clean up now we are not providing junit output.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review

Removed as suggested.

bbrala’s picture

bbrala
Location Netherlands
commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes

Dependency evaluation; all these are dev dependencies that do a single small task of converting tool output to a format GitLab can read, none of this is needed at runtime and will in effect only be executed on GitLab.

micheh/phpcs-gitlab

Release cycle: two releases in 2020, none since; both the plugin and the GitLab output format are stable and seemingly require no maintenance.

Security policy: none

Maintainer: Michel Hunziker https://github.com/micheh

eslint-formatter-gitlab

Release cycle: major releases approximately once a year, follows strict semver and updates when NodeJS is updated.

Security policy: none

Maintainer: Remco Haszing https://gitlab.com/remcohaszing

stylelint-formatter-gitlab

Release cycle: three releases in four years

Security policy: none

Maintainer: Leonid Meleshin https://gitlab.com/leon0399

catch’s picture

catch
he/him
Primary language English
commented

Needs a cspell change but otherwise looks good. Presumably adding this in the main yarn build will help with caching on gitlab a bit too.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks great to me. Going to make a massive difference to contributor experience.

  • catch committed 8224c90c on 11.x 
    Issue #3387339 by bbrala, longwave: Integrate codequality reports into...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.1.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 11.x, thanks!

This doesn't cherry-pick cleanly to 10.1.x due to the yarn.lock changes - we'll need to decide whether to backport it or have it as 10.2.x-only. Might be better to backport in order to keep the yaml in sync so moving back for now.

Added the dependency evaluation to the issue summary.

bbrala’s picture

bbrala
Location Netherlands
commented

Yay it's in!

Backport will be a good idea imo. Not sure what the besta pproach for that would be though.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It caused regression in CI

stylelint and eslint jobs (which are using to override variables) are using PHP 8.1 image

- https://git.drupalcode.org/project/drupal/-/jobs/104537
- https://git.drupalcode.org/project/drupal/-/jobs/104535
vs
- https://git.drupalcode.org/project/drupal/-/jobs/104536
- https://git.drupalcode.org/project/drupal/-/jobs/104538

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 10.1.x-dev » 11.x-dev
Status: Patch (to be ported) » Needs work

I mean the following section resetting _TARGET_PHP variable

+  variables:
+    ESLINT_CODE_QUALITY_REPORT: ../eslint-quality-report.json
fjgarlin’s picture

fjgarlin commented

Correct, it's completely overriding this section:

.default-job-settings: &default-job-settings-lint
  ...
  variables:
    _TARGET_PHP: "8.2"
    _TARGET_DB: "mysql-8"

We could just add ESLINT_CODE_QUALITY_REPORT and STYLELINT_CODE_QUALITY_REPORT to the default-job-settings-lint template and remove those overrides from '🧹 CSS linting (stylelint)': and '🧹 JavaScript linting (eslint)':.

Or we could use the !reference [.default-job-settings, variables] syntax in the sections that override the variables.

I think the first solution might be easier/cleaner.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think we should not set those variables at all; the docs for both packages imply the variable is optional and it will try to discover the location from the pipeline YAML.

longwave opened merge request !4840

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Opened MR!4840 to try this.

fjgarlin’s picture

fjgarlin commented

Even better! Keeping an eye on the pipeline. If it works RTBC for the changes from my side.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I thought the code didn't take the working directory into account, but it appears that it does, so trying without the export to see if autodetection does work.

fjgarlin’s picture

fjgarlin commented

stylelint-quality-report.json: found 1 matching artifact files and directories and eslint-quality-report.json: found 1 matching artifact files and directories and the images are the php-8.2

with the changes in the MR. The only thing now is removing the KUBERNETES_CPU_REQUEST as this will be fixed upstream and is not needed in this MR.

RTBC when that line goes away.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Reverted, this may fail on GitLab CI because of the (unrelated) runner issue.

bbrala’s picture

bbrala
Location Netherlands
commented

We can inline, but i've looked at this also in another issue and we should probably decouple the vars. I'll search for my solution

Somethign like:


.default-job-variables: &default-job-variables-lint
  _TARGET_PHP: "8.2"
  _TARGET_DB: "mysql-8"

.default-job-settings: &default-job-settings-lint
  interruptible: true
  allow_failure: false
  variables:
    <<: *default-job-variables-lint
  image:
    name: $_CONFIG_DOCKERHUB_ROOT/php-$_TARGET_PHP-apache:production
  rules:
    - if: $CI_PIPELINE_SOURCE == "push" && $CI_PROJECT_ROOT_NAMESPACE == "project"
    - if: $CI_PIPELINE_SOURCE == "merge_request_event"

  variables:
    <<: *default-job-variables-lint
    STYLELINT_CODE_QUALITY_REPORT: ../stylelint-quality-report.json

That should allow merging normally.

But the quick fix is still just the export inline.

bbrala’s picture

bbrala
Location Netherlands
commented

I committed that in #3388162: Investigate realistic resource requests for core jobs, but haven't run the pipeline yet.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We don't even need the export inline, both packages correctly autodetect the path as seen in the last-but-one successful run.

fjgarlin’s picture

fjgarlin commented
Status: Needs review » Reviewed & tested by the community

Yup, RTBC based on that indeed. Lines aren't needed at all.

  • catch committed fe938f57 on 11.x 
    Issue #3387339 by bbrala, longwave, catch, fjgarlin: Integrate...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 11.x, thanks! Leaving open for backport again.

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.2.x-dev
Status: Patch (to be ported) » Fixed

10.2.x was branched since this landed, and 10.1.x is security-only, so we can just leave this in 10.2.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.