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

Update RTD config #483

Closed
wants to merge 1 commit into from
Closed

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jun 20, 2023

Superseded by #485

As noted by Xiaoyang on Slack, it's not intuitive to choose [tests] as the extra requirement for building the docs when we also have [docs]. This PR replaces the selection hoping it works out of the box. In the same process, it renames the config file to comply with current recommendations.

How to review

  • Read the diff and note that the CI checks all pass.
  • Build the documentation and check that it works as usual.

PR checklist

@glatterf42 glatterf42 self-assigned this Jun 20, 2023
@glatterf42 glatterf42 added the docs Documentation, tutorials, etc. label Jun 20, 2023
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #483 (8a680d1) into main (f348a6b) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #483   +/-   ##
=====================================
  Coverage   98.5%   98.5%           
=====================================
  Files         42      42           
  Lines       4489    4489           
=====================================
  Hits        4425    4425           
  Misses        64      64           

@glatterf42
Copy link
Member Author

As per the instructions for checking the RTD build locally, the command completed successfully. We might still be missing some packages required for building the docs by only requiring the extra [docs], but I'm not sure how to test that since the PR originates from my fork (so we cannot simply enable builds for this branch on RTD itself).

@glatterf42 glatterf42 requested a review from khaeru June 21, 2023 09:52
@khaeru
Copy link
Member

khaeru commented Jun 26, 2023

We might still be missing some packages required for building the docs by only requiring the extra [docs], but I'm not sure how to test that since the PR originates from my fork (so we cannot simply enable builds for this branch on RTD itself).

RTD did not originally support building docs for PRs, but has recently added this feature: https://docs.readthedocs.io/en/latest/guides/pull-requests.html. We could turn this on, but should first consider security implications, which I haven't had time to do. (For example: code examples in the documentation or code in Sphinx conf.py can be run while the Sphinx build occurs. Could a user abuse this to, e.g. attempt to DDoS our or someone else's public-facing servers?)

So some options would be:

  1. Cautiously or temporarily (just for this PR) enable that.
  2. Re-open the PR with the same commit pushed to a branch in the main repo.

If (2), then see #484 (comment) and the comment linked from there.

@glatterf42
Copy link
Member Author

From these options, I would choose 2 since I don't know how to assess DDoS potential. However, I'm not sure what I can gain from the linked comment: do you suggest to merge a PR with the suggested changes before opening a new one here?

@khaeru
Copy link
Member

khaeru commented Jun 26, 2023

However, I'm not sure what I can gain from the linked comment: do you suggest to merge a PR with the suggested changes before opening a new one here?

I meant that, if/when you push to a new branch in the main repo and open a new PR, it could be convenient to add 1 more commit adding the 1 suggested line, at the same time.

@glatterf42 glatterf42 mentioned this pull request Jun 29, 2023
2 tasks
@glatterf42 glatterf42 closed this Jun 29, 2023
@glatterf42 glatterf42 deleted the update/rtd-config branch June 29, 2023 11:33
@glatterf42
Copy link
Member Author

Superseded by #485

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation, tutorials, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants