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

Upload docs build in CI #314

Merged
merged 2 commits into from
Jul 10, 2023
Merged

Upload docs build in CI #314

merged 2 commits into from
Jul 10, 2023

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Jul 10, 2023

This is useful to iterate on docs and debug CI issues.

The artifact will be uploaded to GitHub Actions like this:

Screenshot 2023-07-10 at 7 56 47 AM

@@ -37,6 +37,13 @@ jobs:
shell: bash
run: |
tox -edocs
- name: Upload docs artifact
if: always()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason you like the if statement here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like, where it is visually located? I copied the style you used from the Deploy docs job below it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just remove the if statement here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We've found it useful to set if: always. When the Sphinx doc build fails due to warnings (e.g. bad indentation), it still produces the docs artifact. It is useful to be able to look at those docs to understand Sphinx's notoriously cryptic error messages.

@garrison
Copy link
Member

garrison commented Jul 10, 2023

This is useful to iterate on docs and debug CI issues.

Can you give an example of a use case? Is this artifact only useful for PRs, or is it useful for all docs builds?

EDIT: I am starting to understand more. The if: always() means it will upload this artifact even when it fails. So yes, does make sense on every run.

Copy link
Collaborator

@caleb-johnson caleb-johnson left a comment

Choose a reason for hiding this comment

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

LGTM thanks 👍

I'll let @garrison take a look before approving

@Eric-Arellano
Copy link
Collaborator Author

Is this artifact only useful for PRs, or is it useful for all docs builds?

Yeah, it's useful for all CI builds. And there is little downside to this PR other than the couple lines of extra code. The upload should be fast and your docs are only 25MB.

Concrete example: we make a big styling change in #313. I want to make it easy for us all to see what the new docs will look like, without making you build the docs locally.

@garrison garrison enabled auto-merge (squash) July 10, 2023 14:08
@garrison garrison merged commit 34b9fea into Qiskit:main Jul 10, 2023
@Eric-Arellano Eric-Arellano deleted the docs-artifact branch July 10, 2023 14:49
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