Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GitLab Code Climate report #3749

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

KamiYang
Copy link

Adds a new reporter that complies with the GitLab subset of the CodeClimate spec for GitLab CI. Using the report in the specific format enables the use of GitLabs Code Quality widget. This PR is not actually related to #2813 because this PR only adds the GitLab subset and GitLab does not require the use of CodeClimate itself

@herndlm
Copy link

herndlm commented Feb 19, 2023

Nice, perfect timing! I was just looking for something like that.

}//end foreach
}//end foreach

echo $messages;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding (I'm not at all accustomed to phpcs internals btw) - you have to JSON encode each error on its own and use string concatenation because the reporter is expected to print here, right? Which is why json encoding an array of errors does not work and why generate() below needs to add the JSON array brackets I suppose?

@jrfnl
Copy link
Contributor

jrfnl commented Mar 2, 2023

Without an opinion on the report itself: why do you think this report should be hosted in this repo ?

You may not be aware of this, but Reports can be hosted in external packages. Also see: #1948

@Chi-teck
Copy link

why do you think this report should be hosted in this repo ?

What repo it should belong then?
I think coding standards and reports should not be coupled.

PHPStan offers GitLab reporter out of box.

@jrfnl
Copy link
Contributor

jrfnl commented Nov 28, 2023

why do you think this report should be hosted in this repo ?

What repo it should belong then? I think coding standards and reports should not be coupled.

I'd suggest maybe a dedicated package for this report only ? Or possibly for a variation of CI specific reports.

@Chi-teck
Copy link

Just came across this package. Didn't test it myself yet.
https://github.com/micheh/phpcs-gitlab

@soullivaneuh
Copy link

Without an opinion on the report itself: why do you think this report should be hosted in this repo ?

I maybe have an answer to convince you to accept that kind of proposition.

First, the tool already support another kind of report, such as junit. So why junit and not CodeClimate or GitLab code climate ? This is more an interrogation here. :-)

Then, I have a concrete case to expose. I have a GitLab CI project gathering recipes to implement different linters/fixers tools onto standalone CI jobs.

The end user, so the developer, MIGHT want to integrate external standard with composer.
To have my CI job working on such a context, I have to require him/her to declare your tool as a project dependency alongside the standard he/she needs.

Currently, I am using the "provided out of the box" junit report, which is working flawlessly but create an integration widget for automated test report, which is not really appropriate.

image

If I want to move the report to codequality, then I need to create/use an external package by the following methods:

  1. Adding it to the project dependencies, using composer require during the CI job execution.
  2. Ask the end user to add this package to the project dependencies and fail the job otherwise.

In both case, it is not a great solution to me because it make the CI job configuration too intrusive, regardless the potential additional dependencies conflicts it may introduce.
Currently, I preferred to stick to the junit report because it does not worth to force that kind of setup to just moving to a more appropriate report widget. Furthermore, the widget is a paid feature of GitLab. So I MIGHT even force a user to install an additional package for... nothing. 🙃

By the way, I tested the micheh/phpcs-gitlab, reported by @Chi-teck, but it does not look to produce a correct report for GitLab. The widget does not display anything, but I didn't go deeper.

However, if you accept an integration inside the project, it will be simple as "Bonjour !", meaning changing the --report-junit option to --report-gitlab or --report-codeclimate without touching anything to the user's project.

If you want to learn more about my problematic and how I did the GitLab CI integration of this tool, please see the following configuration and documentation on that link: https://gitlab.com/ss-open/ci/recipes/-/tree/v3.2.1/stages/lint/php-code-sniffer

Finally, even if I know it is not really an argument, I would like to notify you that some tools are already implementing a gitlab/github report formats. Some sample I am currently using:

Note: If the debate is still actual, it may be preferable to re-open #2813 or create a new issue for that. What do you think?

Thanks for the reading! 👍

@jrfnl
Copy link
Contributor

jrfnl commented Nov 30, 2023

@soullivaneuh None of those are arguments for putting the maintenance burden of this report on PHPCS. I, for one, don't use GitLab, so there would be no way for me to even test the validity of the report or the validity of future changes.

Saying that requiring an extra package is a burden for the end-user is something which I don't consider a valid argument nowadays with Composer making that as easy as can be.

To have my CI job working on such a context, I have to require him/her to declare your tool as a project dependency alongside the standard he/she needs.

That's just not true. Your reporting package would need a (non-dev) dependency on PHPCS, the "end-user", which also uses external standards, shouldn't need to have that dependency.

You could even offer a Docker container/action runner with your package already installed if needs be.

All in all, unless there is significantly more interest in this feature and a willingness from GitLab users to maintain the report, I don't think it has a place in PHPCS itself.

@Veraxus
Copy link

Veraxus commented Jun 25, 2024

FYI, I just worked around this by having PHPCS create a JSON report --report-json=phpcs-report.json and then converting it to Gitlab CI's "Code Quality" (i.e. CodeClimate-lite) format using JQ.

jq '[
    .files | to_entries[] |
    .key as $file |
    .value.messages[] |
    {
        description: .message + " (" + .source + ")",
        check_name: .source,
        fingerprint: ($file + ":" + (.line|tostring) + ":" + (.column|tostring) + ":" + .message + ":" + .source | gsub("[^a-zA-Z0-9]"; "")),
        severity: (if .type == "WARNING" then "major" elif .type == "ERROR" then "critical" else "info" end),
        location: {
            path: ($file | sub("^/builds/DDM/project-templates/template-wp/"; "")),
            lines: {
                begin: .line
            }
        }
    }
]' phpcs-report.json > gitlab-report.json

@soullivaneuh
Copy link

That's just not true. Your reporting package would need a (non-dev) dependency on PHPCS, the "end-user", which also uses external standards, shouldn't need to have that dependency.

You could even offer a Docker container/action runner with your package already installed if needs be.

@jrfnl I might indeed create a docker container containing all the external library, but it does not fit my use case.

I wrote a GitLab CI recipes library that provide tons of pre-configured lint jobs.

For the PHP CodeSniffer tool, this library is not aware at all of which standard extension the end-user need. I may provide the most popular one, but it will never be exhaustive.

To use this recipe library, the user has to list the tool to his project dependencies and the CI job fetch them using composer install.

So no, global installation may not be the solution for all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants