Build: Bug in target_url, failure to add "success" status if no external version exists

[benjaoming]

Fixes #10364

Reviewing

Nothing complicated here, the bug is very obvious: GitHub (and probably also GitLab) failed our API calls because we set a target_url like target_url=/dashboard/test-builds2/version/2102/edit/ and GitHub said: target_url must use http(s) scheme

I added information to our logs that helped resolve this issue.

celery_1       | [warning  ] GitHub commit status creation failed. Unknown GitHub response. [readthedocs.oauth.services.github] build_id=87 builder=09e496859ed3 commit=1cd07f31dcecdcbedbbae5b0997a13879ae7f2f8 commit_status=success debug_data={'message': 'Validation Failed', 'errors': [{'resource': 'Status', 'code': 'custom', 'field': 'target_url', 'message': 'target_url must use http(s) scheme'}], 'documentation_url': 'https://docs.github.com/rest/commits/statuses#create-a-commit-status'} http_status_code=422 integration_type=github_webhook ip=140.82.115.29 parent_task_id=7bbed1ea-d40f-42c3-9f1b-1e9e43ed0631 path_resolved=/usr/src/app/checkouts/readthedocs.org/user_builds/09e496859ed3/test-builds2/checkouts/2102/.readthedocs.yaml project_slug=test-builds2 request_id=500601fc-3801-447e-b65c-d2f0cceff0e9 social_account_id=1 social_provider=github status=success statuses_url=https://api.github.com/repos/readthedocs/test-builds/statuses/1cd07f31dcecdcbedbbae5b0997a13879ae7f2f8 target_url=/dashboard/test-builds2/version/2102/edit/ task_id=f0c811bb-3276-464a-a2f7-6acbee788ce3 user_id=None user_username=x version_slug=2102 webhook_event=pull_request

Curious case

Notice this line in the changeset: if not self.built and not self.uploaded

This was the reason why SOME PRs had "success ✔️" while PRs where the FIRST commit was supposed to be skipped got "pending 🟡" forever.

I haven't changed this, I think it's fine that a skipped build continues to link to the PR preview, otherwise it can confuse users that want to preview the build. Skipping a build is nice, but if it adds extra steps in review work, it will be annoying.

It is of course quite possible that we should open an issue about "Never skip the FIRST commit in a PR". But I think this is again a case of why we shouldn't leave this entire feature for the user to configure but make it an entirely build-in feature.

Q&A

This change is deployed in my local dev environment setup and runs side-by-side with the production env that reproduces the bug.

So this PR basically demonstrates it:

readthedocs/test-builds#2102

I had a look around the codebase for uses of Version.get_absolute_url and it's only used in ways that seem to have the correct output, i.e. does not prepend any additional https:// -- so we won't suddenly have https://domain/https://domain or similar ✔️

[humitos]

Looks good with some changes 👍🏼

I'm concern about breaking the API of get_absolute_url method since there are other places in our code that are expecting it to return without the scheme and domain, just the full path. We should add the scheme and domain from where we are calling this method inside GitHub and GitLab integrations instead of inside get_absolute_url.

[humitos]

I'm not sure that the rename of the state to status helps here. We call it status in some places and GitHub calls it state. Probably makes sense to keep consistency in our code, tho 👍🏼

[benjaoming]

I have changed this because this is definitely about status. GitHub also calls it status AFAICT? https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28

[humitos]

We are sending state in the data we send to them.

[benjaoming]

No, we're sending the status :) I know that it's meaningless to talk about a real difference between these two words, but I think we only use "state" to refer to "build state" and everything else is "status".

I think that clearly when it's called SELECT_BUILD_STATUS[status]["github"], it is correct to call the variable github_build_status and it did make me less confused after I renamed it :)

[humitos]

We send the state to GitHub here https://github.com/readthedocs/readthedocs.org/pull/10369/files/1b1d9fe39fa3de359fb13e9189c3f887b53dab7d#diff-99df3e1a60131d41823464e1ca35dcf068573dffb2ea4ce12dc68968c0ec71beR464

[benjaoming]

Ah okay, my brain thinks about "GitHub commit status" - but I can see they called a field in the API "state", perhaps it was to avoid mixing it up with HTTP status. I think we should try to simplify our terminology rule to saying that we have "build state" for the Build objects internal naming of states - and then we have status for everything else. And then imagining a better future, we do a refactor to stop calling it "Build state" and call that "Build status" as well, so everything is "status" and we don't have "state" suddenly pop up and cause confusion :)

[humitos]

Looks good! Thanks for taking care of this and doing the required work! 💪🏼

[benjaoming]

There are still issues with build skipping.

I think it's related to PRs with only 1 commit. Will reopen the issue related to this change.

[benjaoming]

Example: #10393