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 ability to rebuild a specific build #8227

Merged
merged 19 commits into from
Jun 9, 2021
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 2, 2021

Continuation of #6993. The difference with the other PR is this one creates a new Build object instead of re-using the old one. This reduces complexity by not adding a new special case and also allows us to keep build history giving the users the opportunity to take a look at the differences and understand why the first one failed but the re-triggered one passed.

NOTE you can see what I changed from the original PR starting at this commit 2fe7612

See my comment at #6995 (comment)

Closes #6524

ericholscher and others added 11 commits May 18, 2020 16:24
This is most useful for PR builds,
but I've enabled it for all builds currently.
It might make sense to only enable it for PR builds,
or other builds where we care about rebuilding a specific commit.

Fixes #6524
Co-authored-by: Maksudul Haque <saad.mk112@gmail.com>
Co-authored-by: Maksudul Haque <saad.mk112@gmail.com>
Instead of re-using the `Build` object, we are creating a new one following the
same process that we already have for normal builds.

This reduces the complexity of the code by not adding a new special case and
also keeps the history of build outputs so users can compare it and figure it
out where the potential problem was.
@humitos humitos added the Needed: design decision A core team decision is required label Jun 2, 2021
@humitos
Copy link
Member Author

humitos commented Jun 2, 2021

Marked as Needed: design decision due to my comment at #6995 (comment)

humitos added 2 commits June 3, 2021 12:27
Allowing to re-build all the versions and all the past build could end up on
publishing outdated docs for a stable/latest version. So, we are restricting
this only to External Versions and only to its latest build object.

See discussion in #6995 (comment)
- check at view level (POST)
- test case for the check
@humitos humitos requested review from ericholscher and a team June 3, 2021 10:53
@humitos humitos removed the Needed: design decision A core team decision is required label Jun 3, 2021
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks pretty simple. Only a small code structure question. 👍

readthedocs/builds/views.py Outdated Show resolved Hide resolved
readthedocs/builds/views.py Outdated Show resolved Hide resolved
Using 400 as response results in a blank page. It's better to clearly
communicate what's the problem and redirect to the build listing page.
@humitos humitos requested a review from ericholscher June 7, 2021 10:52
readthedocs/builds/views.py Outdated Show resolved Hide resolved

{# Show rebuild button only if the version is external and it's the latest build for this version #}
{# see https://github.com/readthedocs/readthedocs.org/pull/6995#issuecomment-852918969 #}
{% if request.user|is_admin:project and build.version.type == "external" and is_latest_build %}
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but I remember is_admin: project being expensive, we are using the check 3 times now. We can move it to the context instead.

context['is_project_admin'] = AdminPermission.is_admin(
self.request.user,
project,
)

{# see https://github.com/readthedocs/readthedocs.org/pull/6995#issuecomment-852918969 #}
{% if request.user|is_admin:project and build.version.type == "external" and is_latest_build %}
<div data-bind="visible: finished()">
<form method="post" name="rebuild_commit" action="{% url "builds_project_list" project.slug %}">
Copy link
Member

Choose a reason for hiding this comment

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

Feels weird having this on the same list view, I think we could have another view /builds/{pk}/trigger/ or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a common pattern: POST to a list view to create a new object. We are using this in the same way when triggering a build for a particular version on Build List page at

<form method="post" action="{% url "builds_project_list" project.slug %}">

Seems fine to me to re-use the view and just pass build_pk to trigger a build for a particular version at a specific point in time.

@humitos
Copy link
Member Author

humitos commented Jun 8, 2021

I addressed all the suggestions. This can be re-reviewed and merged now.

@ericholscher
Copy link
Member

Looks good to me once CI passes 👍

@humitos humitos enabled auto-merge June 9, 2021 07:15
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.

Allow to re-trigger a build from the build page
3 participants