search

readthedocs / ext-theme

Read the Docs drop in replacement site templates
2 stars 2 forks source link

Build: rebuild button should include build_pk in POST #336

Closed agjohnson closed 2 months ago

agjohnson commented 5 months ago

At least for PR builds, the rebuild button needs to send over the build_pk as well. Normal versions seem to rebuild fine, but PR builds give a 404 response when the build_pk is missing from the post. Or at least this is the only difference I spot between the beta and legacy dashboards currently, there could be something else happening here.

agjohnson commented 3 months ago

I found out why this is awkward. It is because there are two different "retry" methods used here. For external versions, we evaluate Build.can_rebuild and post to the build detail page. For non-external versions, we aren't retrying a Build instance but instead create a new Build. We post to the project version list URL.

These is also always the APIv3 build trigger API but I don't know if it works for external versions.

agjohnson commented 3 months ago

I finally got set up to test with pull request builds locally. My change does indeed trigger a new build on PR builds, but this shows up as a new build entirely and does not update the existing pull request (at least on GitLab, there is no new pipeline created).

I am starting the new build through APIv3 and the build create endpoint.

To recap what is happening:

I suspect the best change is using the same code in our APIv3 endpoint as the BuildTriggerMixin, so that we can build PR versions as well.

humitos commented 3 months ago

I'm not sure I'm understanding correctly here, but we should always create a new Build object when triggering a new build, no matter what type of version the build is attached to (branch, tag or external)

agjohnson commented 3 months ago

I thought the external version rebuild reused the last build, but it only uses the last build to get the commit to rebuild. This API endpoint was able to rebuild an external version build, but because it didn't previously support fetching the last version build, it was not calling trigger_build(..., commit=build.commit) and the build status was not updating on the PR.

I ended up patching the APIv3 to just do what we need with

humitos commented 3 months ago

but it only uses the last build to get the commit to rebuild

When rebuilding a version, we don't need to grab the commit used at the time the version was built. We should always build using the latest commit for that version. Rebuilding a version using a fixed commit will be pretty confusing and it's not the feature we want to expose to users.

If we were using that before, it's the wrong behavior and it's a bug.

agjohnson commented 3 months ago

When rebuilding an external version we do need to use the latest commit, I'm not describing rebuilding standard versions above. This logic comes directly from existing code:

https://github.com/readthedocs/readthedocs.org/blob/488d50dc4239d3d73ccf9d142f009714ebc95e82/readthedocs/builds/views.py#L58-L82

Seems this comes from at least in part: