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

Testing readthedocs' pr integration (do not merge) #7647

Closed
wants to merge 1 commit into from

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Sep 14, 2021

No description provided.

@fgaz
Copy link
Member Author

fgaz commented Sep 14, 2021

@fgaz fgaz closed this Sep 14, 2021
@fgaz fgaz deleted the fgaz/rtd-integration-test branch September 14, 2021 09:17
@Mikolaj
Copy link
Member

Mikolaj commented Sep 14, 2021

Yay! How do we wire it into CI? Or do we keep the new CI docs check in CI and use this one only for people that want to see the actual results of doc changes? Does it email somebody when it fails or just show a broken page?

@fgaz
Copy link
Member Author

fgaz commented Sep 14, 2021

We could mark it as required and remove the github actions one. Before that we'd have to reflect the flags from the makefile to readthedocs, eg. https://docs.readthedocs.io/en/stable/config-file/v2.html#sphinx-fail-on-warning

Does it email somebody when it fails or just show a broken page?

it shows a failed check in the CI section like github actions (it's hidden now since I closed the pr, but you can see it by clicking the red X on the commit)

@Mikolaj
Copy link
Member

Mikolaj commented Sep 14, 2021

Got it. That's great news. Let's keep them both until we are sure they work consistently and the check by @andreasabel is completely subsumed by the ultimate check using Read The Docs.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 20, 2021

Or does #7638 introduce more/stricter checks and they can't be ported to readthedocs server proper? Or can/should it introduce stricter checks? Is it worth keeping another CI job?

@fgaz
Copy link
Member Author

fgaz commented Sep 20, 2021

I don't know yet, I didn't check in depth

@andreasabel
Copy link
Member

@fgaz You could try to run it on a PR that broke links in the docs, e.g., #7498.
To prevent doc formatting issues (broken links etc.) I think there should be a CI check that fails in that case.

@andreasabel andreasabel added continuous-integration re: readthedocs Concerning hosting documentation on `readthedocs` labels Oct 27, 2021
@andreasabel
Copy link
Member

andreasabel commented Oct 27, 2021

This conversation ends abruptly. The ReadTheDocs CI seems to be running, see https://readthedocs.org/projects/cabal/builds/15099302/, but where is it documented, like:

Please add pointers here.

@fgaz
Copy link
Member Author

fgaz commented Oct 27, 2021

how to switch it on/off
how to maintain

like all jobs, its settings are managed upstream (in this case here and in .readthedocs.yml) and it can be marked as required in the repo settings

why is it showing up in the list of checks in a PR, but not here: https://github.com/haskell/cabal/actions

only github actions show up in that page

@Mikolaj
Copy link
Member

Mikolaj commented Oct 27, 2021

I think, given the current breakage of the readthedocs CI job, the importance of having a local docs test is underlined. In addition, it has stricter settings and just a slightly different angle and context. Also, it runs only when /doc/ changes, so is very cheap compared to the readthedocs job. I'm in favour of having both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous-integration re: readthedocs Concerning hosting documentation on `readthedocs`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants