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 Reporter #4288

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@hco
Copy link

hco commented Jan 31, 2019

Gitlab does support a subset of the CodeClimate JSON Format.
I initially called the Reporter "CodeClimate", but that would "require" more fields that we currently cannot fill appropriately from a report generator, I think.
So I decided to rename it to "Gitlab Reporter".

It will result in a pull request annotation like this:
image

I will be happy to maintain this reporter, if problems arise.

In a next step I would be happy to add meaningful line numbers, if someone can assist me on that, but after reading #3601 it does not seem to be so easy, and I think the current state is enough for an "MVP" ;-)

@hco hco force-pushed the hco:feature-gitlab-reporter branch from d2bbd34 to d8d160e Jan 31, 2019

@hco hco force-pushed the hco:feature-gitlab-reporter branch from d8d160e to 00657d5 Jan 31, 2019

@keradus

This comment has been minimized.

Copy link
Member

keradus commented Jan 31, 2019

Hi @hco , thanks for your PR, idea looks interesting indeed.
Before going into details of the code, we need to ensure that format is valid.
please upload schema of this format to /doc

@hco

This comment has been minimized.

Copy link
Author

hco commented Jan 31, 2019

@keradus I think Gitlab does not provide a schema :-(

Or would a self-written schema help?

@hco

This comment has been minimized.

Copy link
Author

hco commented Jan 31, 2019

Oh - and thanks for the fast feedback!

@keradus

This comment has been minimized.

Copy link
Member

keradus commented Feb 4, 2019

as a first step i would suggest you asking gitlab for schema, maybe it's there, but they forgot to publish it, or simply it's published but we can't find it? it happened before ;)

I'm totally up for having a gitlab support, just let us ensure it works the best and it's stable, meaning we have the schema first.

@keradus

This comment has been minimized.

Copy link
Member

keradus commented Feb 17, 2019

@hco , any update on this ? had you manage to find/ask for gitlab official schema ?

@hco

This comment has been minimized.

Copy link
Author

hco commented Feb 17, 2019

I asked, but got no response yet. :(

@hco

This comment has been minimized.

Copy link
Author

hco commented Feb 20, 2019

So the current state is that they are pointing me to code climate, which would require us to generate more fields, which are not necessary for supporting gitlab.
In addition, code climate does not currently provide an official schema. Asked them as well.

Anything I can do while we wait?

@hco

This comment has been minimized.

Copy link
Author

hco commented Mar 7, 2019

No response from Code Climate yet.

I currently think that the only think I could do is write my own schema for GitLab-CI.
Do you think that this would give us more benefit than the existing tests? 🤔

Do you have other suggestions on what I should do?

The Travis build failure is nothing I need to work on, right?

@SpacePossum SpacePossum added this to the 2.15.0 milestone Mar 14, 2019

hco added some commits Mar 14, 2019

@hco

This comment has been minimized.

Copy link
Author

hco commented Mar 14, 2019

Done. Thanks for the feedback!

The change did not involve any test, though. I think this is outside of the scope of this PR, but maybe there should be a testcase added for this to the AbstractReporterTestCase, right?

If you agree, I can tackle that later aswell.

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Mar 15, 2019

Thanks for the changes, this PR looks good to me :)

If you've some time for a test for the AbstractReporterTestCase please give it a shot. I remember hunting down the not-escaped bug on the previous reporters, so if we can catch it in the future by a test it would great. For me it is not blocking for this PR though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.