-
Notifications
You must be signed in to change notification settings - Fork 276
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
turn doc build warnings into errors #1724
turn doc build warnings into errors #1724
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think this is a good change and we should keep it, we have the docs environment in tox only for local builds. In the CI we use the integrated webhooks from readthedocs.
So in addition to your change, we need to update the .readthedocs.yaml file with the fail_on_warning option: https://docs.readthedocs.io/en/stable/config-file/v2.html#sphinx
Pull Request Test Coverage Report for Build 1586677835
💛 - Coveralls |
Thank you @sechkova, I see now the failure in the checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me.
Well CI shows it failing as intended... I'm not sure about this though: reproducing tests locally is pretty important (sending PRs only to get red lights is demoralizing) and our testing setup is already a bit complicated in that regard: Could we set default The CI already shouldn't need the envlist as I think it it explicitly sets toxenv to |
+1 to set default |
Yeah that sounds good to me. build fix is in (#1726), if you rebase you should also get a green build now... |
aee4963
to
0fe0b8b
Compare
I do, yes. But I'm also fine in changing the way I invoke tox to do it. Given this change we should also update the note about tox in the contributors docs (see bottom). But we can add that to #1709. @kairoaraujo, would you mind adding a note there? |
I don't mind adding a note there, but can someone help me with some decisions? Current doc:
My initial suggestion is:
About the part that mentions tox will install tuf's "dependencies anew in each environment.". Some suggestions:
Should I also add a piece of information about using an alternative installed Python version? Suggestion
|
As general advice: We should aim to make the document (quite a bit) shorter, not longer. I can try to come up with actual suggestions later but maybe that helps already? |
This commit adds to the docs tox session the flag ``-W``, which turns the warnings into errors. The CI will fail once it gets errors. Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
CI uses the integrated webhooks from readthedocs. Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
Simplified ``tox`` environ list not to expect that developer has all multiple Python version, but instead run with the python version available. Also, it adds docs build to environ list. Running ``tox`` will run the lint, docs, and py. The CI covers the multiple supported Python versions, and the developers still can use tox -e py{version} Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
The mention of testing using multiple Python versions by default was removed. Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
0fe0b8b
to
7dc5940
Compare
I added a small and simple suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
This commit adds to the docs tox session the flag
-W
, whichturns the warnings into errors.
The CI will fail once it gets errors.
Signed-off-by: Kairo de Araujo kdearaujo@vmware.com
Linked to #1722