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 support #192

Merged
merged 11 commits into from
Dec 1, 2020
Merged

Add gitlab support #192

merged 11 commits into from
Dec 1, 2020

Conversation

agamm
Copy link
Contributor

@agamm agamm commented Nov 12, 2020

Fixes #191

@coveralls
Copy link

coveralls commented Nov 12, 2020

Coverage Status

Coverage decreased (-0.1%) to 10.978% when pulling 7595826 on agamm:add-gitlab-support into f1c5f0d on mattn:master.

Copy link
Collaborator

@gdm85 gdm85 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR!

I am requesting some changes here and also I think the CI_MERGE_REQUEST_ID or CI_MERGE_REQUEST_IID should be covered to get the Merge Request ID (corresponding to PR number)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
goveralls.go Show resolved Hide resolved
@agamm
Copy link
Contributor Author

agamm commented Nov 15, 2020

Thanks for submitting this PR!

I am requesting some changes here and also I think the CI_MERGE_REQUEST_ID or CI_MERGE_REQUEST_IID should be covered to get the Merge Request ID (corresponding to PR number)

I didn't really understand what you meant by: CI_MERGE_REQUEST_IID, did my changes relate? Also I didn't find where to test this part of the code.

@agamm agamm requested a review from gdm85 November 15, 2020 13:20
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@gdm85
Copy link
Collaborator

gdm85 commented Nov 15, 2020

I didn't really understand what you meant by: CI_MERGE_REQUEST_IID, did my changes relate? Also I didn't find where to test this part of the code.

Your PR introduces use of CI_EXTERNAL_PULL_REQUEST_IID for the PR number; that is only valid when building a GitHub project on GitLab (see https://docs.gitlab.com/ee/ci/variables/predefined_variables.html). If you are building a MR on GitLab using a GitLab project, then you need either CI_MERGE_REQUEST_ID or CI_MERGE_REQUEST_IID (I suspect the latter).

Can you please verify?

agamm and others added 2 commits November 15, 2020 18:52
Co-authored-by: gdm85 <gdm85@users.noreply.github.com>
Co-authored-by: gdm85 <gdm85@users.noreply.github.com>
@agamm
Copy link
Contributor Author

agamm commented Nov 15, 2020

CI_MERGE_REQUEST_ID

I work by mirroring my github to gitlab so I can't really verify that super easily. What did you have in mind, I'll do my best?
PS it seems that CI_MERGE_REQUEST_ID is more relevant. ("unique ID across all projects on GitLab")

@agamm
Copy link
Contributor Author

agamm commented Nov 15, 2020

I created a temporary repo to test this fork, I always got an ambigious message of a 500 error:

Bad response status from coveralls: 500
{"message":"Build processing error.","error":true,"url":""}

Pipeline example: https://gitlab.com/agmore1/testgoveralls/-/jobs/850406137
.gitlab-ci.yml: https://gitlab.com/agmore1/testgoveralls/-/blob/master/.gitlab-ci.yml

@gdm85
Copy link
Collaborator

gdm85 commented Nov 17, 2020

I created a temporary repo to test this fork, I always got an ambigious message of a 500 error:

Bad response status from coveralls: 500
{"message":"Build processing error.","error":true,"url":""}

Pipeline example: https://gitlab.com/agmore1/testgoveralls/-/jobs/850406137
.gitlab-ci.yml: https://gitlab.com/agmore1/testgoveralls/-/blob/master/.gitlab-ci.yml

That sounds like a bug in coveralls.io itself; cc @nickmerwin

@agamm please update your project to use go get github.com/mattn/goveralls@master and then run it with with -debug=true, that might provide interesting clues for troubleshooting the server-side bug.

I work by mirroring my github to gitlab so I can't really verify that super easily. What did you have in mind, I'll do my best?
PS it seems that CI_MERGE_REQUEST_ID is more relevant. ("unique ID across all projects on GitLab")

I think what we need here is the MR ID, so that coveralls.io can link back to GitLab with the right reference. Similarly, for GitHub it's the PR number not the unique PR ID across all projects.

@agamm
Copy link
Contributor Author

agamm commented Nov 19, 2020

-debug=true

I tried updating from master but there were no new changes, I did change the merge request id to iid like you said, although I still honestly don't understand that part.

Seems to still fail:
https://gitlab.com/agmore1/testgoveralls/-/jobs/860029893

@agamm
Copy link
Contributor Author

agamm commented Nov 29, 2020

@gdm85 any update?

@gdm85
Copy link
Collaborator

gdm85 commented Dec 1, 2020

@agamm that 500 is a bug on coveralls.io side; you can try using -shallow but they need to fix it on their side.

@gdm85 gdm85 merged commit f28e79e into mattn:master Dec 1, 2020
@afinetooth
Copy link

FYI @agamm and @gdm85, on behalf of coveralls, we provided a workaround for the current issue that entails adding the commit-sha to the base of your posted json. However, a fix will go to production in 24-48 hrs so please try again and let us know if it still occurs. thanks.

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

Successfully merging this pull request may close these issues.

How can I support Gitlab CI?
4 participants