Skip to content

Conversation

@daspecster
Copy link
Contributor

@dhermes what do you think of moving the documentation generation to after_success?

Fixes #2121

@daspecster daspecster added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. docs labels Aug 18, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 18, 2016
@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

If you put it in after_success it won't fail the build if there's a problem

@daspecster
Copy link
Contributor Author

@dhermes switched them back around. If I understand correctly though, even if the unittests fail, the docs will still get deployed. I don't know if that's the best case.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

I don't know if that's the best case.

It's not. Are you deploying on PRs or just merges?

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

You'll have to apply the **kwargs fix from #2071

@daspecster
Copy link
Contributor Author

Yup, thanks!

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

I assume / hope the Yup wasn't for "Are you deploying on PRs or just merges?"

@daspecster
Copy link
Contributor Author

Apparently it is! I'm fixing that right now.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

Oy

@daspecster
Copy link
Contributor Author

daspecster commented Aug 18, 2016

How does tox -e docs deploy to RTD? Is that just from the READTHEDOCS envar?

Update: nm I see the docs-rtd now.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

It doesn't, https://readthedocs.org/projects/gcloud-python/ looks for commits to master and triggers a build.

The relevant file was scripts/update_docs.sh before you implemented the new docs env

@daspecster
Copy link
Contributor Author

A while ago, I made a separate script for the JSON docs, update_json_docs.sh. Travis is calling that now instead of tox -e json-docs directly.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

@daspecster
Copy link
Contributor Author

I guess I had more coffee in me back then.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

It doesn't look like update_json_docs.sh checks if the docs build during a PR

@daspecster
Copy link
Contributor Author

daspecster commented Aug 18, 2016

That should run it every time, or were there some cases that you didn't want the docs to build? Also, should I have it build the RTD docs with tox -e docs?

I added that line as well.

}

# Run this to verifiy that the docs build successfully.
tox -e docs

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

tox -e docs has been changed by you (previously) to update_json_docs.sh, which now calls tox -e docs, thus creating an infinite recursion.

tox -e docs-rtd makes sure the docs actually build with Sphinx (which is the concern)

@daspecster
Copy link
Contributor Author

daspecster commented Aug 18, 2016

I didn't change tox -e docs, I added tox -e json-docs.

[testenv:docs]
https://github.com/GoogleCloudPlatform/gcloud-python/blob/master/tox.ini#L67

[testenv:json-docs]
https://github.com/GoogleCloudPlatform/gcloud-python/blob/master/tox.ini#L79

So I think it should be ok?

Except for my refactor now failing.

generate_json_docs.py: error: argument --tag: expected one argument

@daspecster
Copy link
Contributor Author

buildDocs $TRAVIS_BRANCH

if [ "${TRAVIS_BRANCH}" == "master" ] && [ "${TRAVIS_PULL_REQUEST}" == "false" ]; then
buildDocs $TRAVIS_BRANCH

This comment was marked as spam.

This comment was marked as spam.

}

# Run this to verifiy that the docs build successfully.
buildDocs $TRAVIS_BRANCH

This comment was marked as spam.

if [[ ! -z ${TRAVIS_TAG} ]]; then
DOC_VERSION = $TRAVIS_TAG
else
DOC_VERSION = $TRAVIS_BRANCH

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

@daspecster This LGTM except for the tiny nit about using ${...}. Please squash and rename the PR to actually state what is happening

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

You can merge once you've fixed the nit and Travis goes green, no need to check with me

@daspecster daspecster changed the title Add RTD doc build, and move to after_success. Allows build to fail if JSON docs fail to generate. Aug 18, 2016
@daspecster daspecster merged commit 78ba81e into googleapis:master Aug 18, 2016
@dhermes dhermes mentioned this pull request Sep 19, 2016
@daspecster daspecster deleted the fix-rtd-builds branch January 24, 2017 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants