Reporting build status to Gitlab Merge Request doesn't work for forks #7370

Open maxking opened 4 years ago

Details

I have enabled building PRs for our Gitlab project (https://gitlab.com/mailman/mailman) and have gone through all the steps to connect the Gitlab account in my Readthedocs account. However, the projects that I have imported are using 'mailman' gitlab gruop, that I have admin rights on.

However, I always get

 Could not send GitLab build status report for Mailman Core. Make sure you have the correct GitLab repository permissions and your GitLab account is connected to ReadtheDocs.

When I visit readthedocs.org.

Read the Docs project URL: https://readthedocs.org/projects/mailman/
Build URL (if applicable): https://readthedocs.org/projects/mailman/builds/11618093/
Read the Docs username (if applicable): maxking
URL for the Pull Request: https://gitlab.com/mailman/mailman/-/merge_requests/680

Expected Result

I expected the build results to be reported via Pipelines.

Actual Result

Nothing is reported in the pipelines page, although, the Build does get triggerred on Readthedocs and documentation is available to browse if you manually visit the builds page and get the URL to the PR docs.

Are GitLab groups the same as organizations in GitHub? or are they a subgroup of organizations?

Ok, I think this is bug from our side, we received the merge request event, but we processed it like a push event instead of a merge request event.

My above comment isn't right, the merge request was closed, so it was processed according to that. Looks like builds are working, but the status isn't being reported. I'll dig more into this.

Are GitLab groups the same as organizations in GitHub? or are they a subgroup of organizations?

They are like organizations on Github. You can see the one we are using at https://gitlab.com/mailman

And yes, the builds are indeed working. It is just the status that isn’t being reported.

btw, your project is building from https://gitlab.com/mailman/mailman-suite-doc/-/merge_requests, the MR you linked seems from another project.

I checked the logs, and we are sending the status for some projects https://gitlab.com/daltonproject/daltonproject/-/merge_requests/140. Your project is failing here

https://github.com/readthedocs/readthedocs.org/blob/7274123c2c970e70d79542aba969c38864d9a376/readthedocs/oauth/services/gitlab.py#L547-L553

Well, the project is Mailman Core, which is a sub-project of Mailman Suite. All the builds for Mailman Core project are in RTD at https://readthedocs.org/projects/mailman/builds/ and the corresponding merge requests are at https://gitlab.com/mailman/mailman/-/merge_requests.

I am sorry I gave you a wrong project url, it should be https://readthedocs.org/projects/mailman/. (I’ll edit the issue description too).

Also, thanks for the pointer to the code where it fails. Seems like Gitlab is retuning permission errors indeed, and I am not sure if that is because the scope of the token requested by RTD doesn’t cover the project that it is trying to send status for or something else. I do have owner permissions on all the projects under Mailman group on Gitlab. I’ll also look up some of the Gitlab docs to see if there is scope an issue.

It seems like the api access level, which RTD requests and has access to, should grant it ability to work with the groups according to their docs.

It would be good to know the exact status code of the response between 401, 403 and 404. My suspicion is more on 404 and not 403 because looking a the code, the URL that you POST the status to url}/api/v4/projects/{repo_id}/statuses/{commit} would only work if the PR was created using the branch on the main repo.

For forks, which is true in my case, repo_id would be different and POSTing the status would result in 404 since the {commit} doesn’t exist in the main repo that RTD is configured with.

There are two ways to go about this:

You try to get the repo_id of the fork the MR was created from and try to POST the status to that repo, which would fail in most cases since the user whose access token RTD holds, will not have access to any other forks other than their own.
You simply do not send build status for the MRs created from forks, which you can do by comparing the source and target in the webhook event that you get from Gitlab.

It would also be nice to not show a warning in RTD in this particular situation that it failed to POST a response due to auth errors since that simply leads the users to believe it can be fixed by re-linking Gitlab account and syncing webhooks.

So, the message you're seeing in the dashboard is created when you sync/create the webhook, it disappears by clicking on it.

For forks, which is true in my case, repo_id would be different and POSTing the status would result in 404 since the {commit} doesn’t exist in the main repo that RTD is configured with.

Ok, that's interesting I'll look more into that.

About the status code, I just merged a PR to have more information in the logs https://github.com/readthedocs/readthedocs.org/pull/7385, it will be live by next week.

I sort of verified my theory by simply create a PR from the same main repo and seeing if RTD is able to POST the status back to Gitlab and it does.

See MR: https://gitlab.com/mailman/mailman/-/merge_requests/683 and RTD build at: https://readthedocs.org/projects/mailman/builds/11658674/

And the warning that I was talking about is this, which is created after every build that it couldn’t POST the response to when you login to your RTD account. It would be nice if this happened only when the status was 403.

Thanks for the additional info! So, when a project isn't public a 404 can be returned, so that's why we catch those too.

I think this is a issue with GitLab. More details can be found here: https://gitlab.com/gitlab-org/gitlab/-/issues/27636

readthedocs / readthedocs.org