Fix status reporting on PRs with the magic exit code

[ericholscher]

This was because we were using this logic to return a relative URL,
and then sending it to GitHub:

• readthedocs.org/readthedocs/oauth/services/github.py

  Lines 459 to 460 in f86b537

  	if not link_to_build and state == BUILD_STATUS_SUCCESS:
  	target_url = build.version.get_absolute_url()

• readthedocs.org/readthedocs/builds/models.py

  Lines 343 to 349 in f86b537

  	if not self.built and not self.uploaded:
  	return reverse(
  	'project_version_detail',
  	kwargs={
  	'project_slug': self.project.slug,
  	'version_slug': self.slug,
  	},

Fixes #9791

[humitos]

This looks good.

I think there are is a little more to dig into here, since it seems we are missing some cases. For example, in this part of the code we are not passing link_to_build=:

readthedocs.org/readthedocs/builds/tasks.py

Line 453 in 90d611d

success = service.send_build_status(build, commit, status)

Also, it seems there is only one case where we want link_to_build=False (when the build completes successfully ¹) and for all the other cases, we want link_to_build=True. However, we are using False as default argument having to change it in most of the cases. Inverting the default, or even better, removing the default and making it always explicit, would be easier to follow and will avoid the first problem that I found while reviewing this code.

Finally, this part of the workflow seems to be broken

readthedocs.org/readthedocs/oauth/services/github.py

Lines 459 to 460 in 90d611d

	if not link_to_build and state == BUILD_STATUS_SUCCESS:
	target_url = build.version.get_absolute_url()

We were hitting it and generating relative URLs which GitHub does not accept that. Since we should never send relative URLs, we should delete that code completely to avoid hitting it again.

...

Aha! You linked to the first part of Version.get_absolute_url! However, the second part, uses a full absolute URL (not relative). See:

readthedocs.org/readthedocs/builds/models.py

Lines 352 to 355 in f86b537

	return self.project.get_docs_url(
	version_slug=self.slug,
	external=external,
	)

Footnotes

1. in that case we link directly to the built documentation from the PR ↩

[ericholscher]

I think I misunderstood this code, so I made it a bit more explicit.

[ericholscher]

@humitos I wonder if this is actually the proper fix after all? Have it match the logic in get_absolute_url?

[humitos]

Hrm, I'm not 100% sure, but I don't think so. .built and .uploaded are not updated to False when a new build is triggered for the same version; so we will be reading an old value here.

IMO, we need this logic:

• Successful build: link to documentation
• Failed build: link to build details' page

As you may noted, the link to send does not depends on the "Commit build status" we sent to GitHub (state == BUILD_STATUS_SUCCESS, in that code), but it depends on the "status" of the build itself.

Actually, I'd completely remove link_to_build and just use the build's status inside this method. That way, we manage the logic just in one place and we don't have to worry about where and why we are passing link_to_build=False/True in different places of the code.

[humitos]

@ericholscher I found this PR while surfing 🏄🏼 . I updated with my proposal that removes link_to_build argument completely and decides linking to docs or build bases on Build.success and Build.state only. I pushed my changes directly to this PR, but we can revert them if you think they are not correct.

[ericholscher]

This is much cleaner and more obvious. I'm 👍 on shipping this!

[ericholscher]

😆