Skip to content

add building of test documentation of RTD #169

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

Closed
wants to merge 5 commits into from

Conversation

basnijholt
Copy link
Member

@basnijholt basnijholt commented Mar 20, 2019

This builds the documentation for each branch on https://adaptive.readthedocs.io/en/test_documentation

Todo:

  • need to check if the build is for the correct Git commit hash here.

@basnijholt basnijholt force-pushed the add_test_documentation branch 7 times, most recently from 48eb673 to 55a444a Compare March 20, 2019 10:12
@basnijholt basnijholt changed the title add building of test documentation of RTD WIP: add building of test documentation of RTD Mar 20, 2019
@basnijholt basnijholt force-pushed the add_test_documentation branch 3 times, most recently from 0943d35 to 82d9b64 Compare March 20, 2019 11:06
@basnijholt basnijholt changed the title WIP: add building of test documentation of RTD add building of test documentation of RTD Mar 20, 2019
@basnijholt basnijholt force-pushed the add_test_documentation branch from 931290c to 81afebb Compare March 20, 2019 11:11
@basnijholt basnijholt changed the title add building of test documentation of RTD WIP: add building of test documentation of RTD Mar 20, 2019
@basnijholt basnijholt changed the title WIP: add building of test documentation of RTD add building of test documentation of RTD Mar 20, 2019
@basnijholt basnijholt force-pushed the add_test_documentation branch 3 times, most recently from 867d27e to eb15f46 Compare March 20, 2019 17:43
@basnijholt basnijholt force-pushed the add_test_documentation branch from eb15f46 to a545197 Compare March 20, 2019 17:47
@basnijholt basnijholt mentioned this pull request Mar 20, 2019
4 tasks
@basnijholt
Copy link
Member Author

This seems to work, @akhmerov, what do you think?

@basnijholt basnijholt mentioned this pull request Mar 21, 2019
@basnijholt basnijholt requested a review from akhmerov March 21, 2019 10:59
@basnijholt basnijholt force-pushed the add_test_documentation branch from a545197 to 86dd87d Compare March 24, 2019 00:43
@akhmerov
Copy link
Contributor

First of all, this is at best a temporary solution because of the following reasons:

  • The test_documentation branch shows on rtd, which may confuse the users.
  • This approach of having one such branch won't work as soon as there's more than one active PR

But even with that in mind, I still don't understand whether this satisfactory. My main question is: what will happen if a user that isn't a team member opens a PR? I can imagine several different possible outcomes:

  • The pipeline doesn't run at all
  • The pipeline runs, but fails because the push fails
  • The pipeline runs and force-pushes to the branch test_documentation in the fork
  • The pipeline runs and force-pushes to the main repository.

In order to come to a decision on this PR I would like to know for sure which of these options is realized. Is there a document that describes the permissions model of azure pipelines with forks?

@basnijholt basnijholt force-pushed the add_test_documentation branch 2 times, most recently from 42f0b67 to 995835a Compare July 10, 2019 17:15
@basnijholt basnijholt force-pushed the master branch 2 times, most recently from 91e38f1 to bc190a7 Compare July 29, 2019 14:13
@jbweston jbweston self-requested a review November 21, 2019 10:22
Copy link
Contributor

@jbweston jbweston left a comment

Choose a reason for hiding this comment

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

I agree with @akhmerov that to merge this we would need to:

  • know what happens when someone opens a PR from their fork of adaptive
  • allow multiple PRs to be open at the same time without each clobbering the other's docs deployment
  • somehow hide all these "test" builds in readthedocs so that it's not confusing for users.

@basnijholt
Copy link
Member Author

@akhmerov and @jbweston, I've opened #262 with a different account to see what happens.

As I expected, no other person will be able to force push to our repo.

Therefore, building the docs will only work for the core maintainers, and since nearly all of the PRs that have ever been made come from us, I believe that that's fine.

I don't think your other point trump the fact that right now we do not have any tests.

Right now, the docs are also broken and it's terrible to debug.

I say, let's merge this as it is!

@akhmerov
Copy link
Contributor

akhmerov commented Apr 8, 2020

It seems that since the initial implementation, rtd has enabled documentation building for PRs: https://blog.readthedocs.com/building-docs-for-pull-requests/, @basnijholt what do you think?

@basnijholt basnijholt force-pushed the add_test_documentation branch from 181b9fd to 20d4c7f Compare April 9, 2020 07:28
@basnijholt
Copy link
Member Author

@akhmerov, I've requested the feature and we've got it!

See for example https://adaptive--263.org.readthedocs.build/en/263/

@basnijholt basnijholt closed this Apr 9, 2020
@basnijholt basnijholt deleted the add_test_documentation branch April 18, 2020 13:51
@basnijholt basnijholt mentioned this pull request May 19, 2020
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.

3 participants