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 #6995

Merged
merged 9 commits into from Jun 9, 2021
Merged

Add ability to rebuild a specific build #6995

merged 9 commits into from Jun 9, 2021

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 29, 2020

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.

It definitely isn't the final UX, but it works reasonably well for now:

Screen Shot 2020-04-29 at 12 20 51 PM

Fixes #6524

@ericholscher ericholscher added the Needed: tests Tests are required label Apr 29, 2020
@ericholscher ericholscher requested a review from a team April 29, 2020 19:19
Copy link
Member

@saadmk11 saadmk11 left a comment

Choose a reason for hiding this comment

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

Its Amazing to see this go through. This will surely help the users a lot. 💯

readthedocs/builds/views.py Outdated Show resolved Hide resolved
readthedocs/core/utils/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

👍 on the feature, but could we either move the button or replace it with a link? This feels a bit unpolished if it's going to be live for all projects.

image

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I'm not sure to understand the use case for this. What are the scenarios where you want to re-build the same commit? What would be different? I can see that the same commit could install different dependencies if they are not pinned for example, but that looks like an edge case to me.

I think the "rebuilding a build that failed because of RTD" --as the image shown, is kind of a hack and we should concentrate on having less of those.

@ericholscher
Copy link
Member Author

I hit this pretty frequently when something weird happens with the PR builder. I think it's a useful enough thing to support, since other ways of supporting this are weird hacks.

ericholscher and others added 5 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>
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Button placement looks good! I'm -1 on repurposing command output for state display, but I think skipping UX on this interaction is absolutely fine. We can revisit polishing this up with new templates.

BuildCommandResult.objects.create(
build=build, command='Rebuilding build', exit_code=0,
start_time=datetime.utcnow(), end_time=datetime.utcnow(),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably remove this entirely, I don't think it's worth introducing UI confusion in the command output. The best place to solve this would be either popping up a confirmation box for the user on click, or simply revert the state back to show the UI as "loading". Either would communicate to the user that the build is restarting

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'm more worried about other folks looking at it and being confused about why it was running again after it had already built. I think we don't do a great job of showing history on builds -- we hit this issue with failures & retries as well, so we need some kind of pattern for it.

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've updated this to use the new build status, which is a better solution 👍

@ericholscher ericholscher removed the Needed: tests Tests are required label May 19, 2020
@ericholscher
Copy link
Member Author

This should be ready for a final review with the new status change 👍

@stale
Copy link

stale bot commented Aug 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Aug 8, 2020
@ericholscher ericholscher requested a review from a team August 8, 2020 15:06
@stale stale bot removed the Status: stale Issue will be considered inactive soon label Aug 8, 2020
@stale
Copy link

stale bot commented Sep 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Sep 26, 2020
@saadmk11 saadmk11 removed the Status: stale Issue will be considered inactive soon label Sep 27, 2020
@ericholscher ericholscher added the Accepted Accepted issue on our roadmap label Sep 28, 2020
@humitos
Copy link
Member

humitos commented Jun 2, 2021

I took a look at this PR today trying to recover the feature and finish it.

@ericholscher is there a strong reason why we are re-triggering the same build object instead of just trigger a new build with a specific commit? IMO, reading the code just makes it more complex passing the build object around different methods, adding a new build status, etc.

I'm thinking that we can just call trigger_build(commit=) with the commit received from the dashboard (via ?commit= or via ?build_pk= if the build is associated with a particular commit). With this approach, it would create a new Build object and we would keep all the build outputs, giving users more context about why the first time it fails and maybe the subsequent re-triggered ones passed.

I'll try opening a PR with this idea.

@humitos
Copy link
Member

humitos commented Jun 2, 2021

I opened a PR at #8227 to show my idea but I'm still not convinced this is a good feature and I think it will create more problems than benefits.

While it may be a good feature to "re-build the latest failed build from a PR", it will for sure create problems for all the other use cases that involve an old commit:

  • re-build an old commit from this external version
  • re-build an old commit from latest
  • re-build an old commit from stable
  • re-build an old commit from any other version

In all these cases, the user will be building and publishing an old version of their documentation, and they may not even notice that they just break their docs. They will end up with that version of their (now out-of-date) docs pointing to an old commit.

Basically, there is going to be a discrepancy between what you have in your repository and what you have published, breaking one of the main benefits of RTD. Read the Docs' case is different from a regular CI because it does not only do checks but also generates and publishes assets.

Considering the best use case for this feature the one I mentioned in the first paragraph, there are currently at least some workarounds that are not ideal but way safer from a user's perspective:

  • add a new commit to the PR, or
  • close and re-open the PR

If any, it seems we should only allow to re-build on that particular case. Still, I don't think it adds a lot of value.

@astrojuanlu
Copy link
Contributor

I agree with the concerns raised by @humitos. I still think this is a good feature though, provided that we narrow down the scope.

  • Reopening and closing the PR also triggers all other CI checks users have. This might be undesirable.
  • The most likely reason to do this is that some temporary network failure ruined the build. Therefore, I don't think users will want to build ancient commits. And if they want, they already have other methods.

Therefore, I think we could limit the feature as follows:

  • Only rebuilds of PR builds are allowed. No danger in rebuilding an old published version, that should be done with the existing UI.
  • Does not allow rebuilding ancient commits. Therefore, we can push a reasonable timeout, say 24 hours.

What do you think? Pinging @pllim , who probably has interest in this (I don't remember if we are discussing the feature in some other issue).

@stsewd
Copy link
Member

stsewd commented Jun 2, 2021

I think the idea was to allow to re-build a PR or version from the latest commit.

@pllim
Copy link
Contributor

pllim commented Jun 2, 2021

24 hours might not work for some. A commit can still be a latest commit but weeks old before a reviewer can get to it and realized a rebuild is needed, though I guess in that case, reviewer can open/close as usual and cancel the other CI (not pretty but it is a workaround).

@astrojuanlu , I do vaguely recall talking about this though what I wanted more is the ability to skip building RTD entirely via [skip ci], [ci skip], [skip rtd], and [rtd skip].

@astrojuanlu
Copy link
Contributor

Ah yes, that's #7660 :)

@ericholscher
Copy link
Member Author

Agreed with @astrojuanlu -- I think we probably only want to allow it for the last build on a PR branch. It should be simple enough to figure this out (pk == Build.objects.filter(version=pr_version).first()). I don't feel strongly about reusing a build object or creating a new one. I initially reused it so that the URL would be the same. I don't think either is obviously better in terms of UX, so whatever is simpler is probably best.

humitos added a commit that referenced this pull request Jun 3, 2021
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)
@humitos humitos merged commit 53cc567 into master Jun 9, 2021
@humitos humitos deleted the rebuild-builds branch June 9, 2021 07:27
@pllim
Copy link
Contributor

pllim commented Jun 9, 2021

I cannot wait to put this into good use. Thanks! 😸

@dimpase
Copy link

dimpase commented Dec 1, 2021

I'm not sure to understand the use case for this. What are the scenarios where you want to re-build the same commit?

In our case we have to rely on our package (https://pypi.org/project/primecountpy/) supplied via a wheel (it depends on an external C++ library, and the rtd builder
cannot handle building the package). Now, our wheels are built on GitHub, using https://github.com/pypa/cibuildwheel, which is triggered by push/PR.
If the wheel is broken, it need not lead to an error in rtd builder; it often passes, but the docs are broken.
So we cannot rely on the rebuild button only available for errored out builds.

Ideally, we would like to be able to install wheels on PyPI and only then trigger the rtd rebuild, but we'd be OK with rebuild
button being available for passed builds (after we install the wheels to PyPI, we can just press it...)

@astrojuanlu
Copy link
Contributor

@dimpase sorry it took so long to respond. Would an incoming webhook suit your use case? You can trigger builds externally https://docs.readthedocs.io/en/stable/integrations.html#using-the-generic-api-integration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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