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.

Sign up for GitHub

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

Jump to bottom

feat: add upload assets to doc-build #467

Merged
merged 27 commits into from
Jun 11, 2024
Merged

feat: add upload assets to doc-build #467

merged 27 commits into from
Jun 11, 2024

Conversation

SMoraisAnsys
Copy link
Contributor

@SMoraisAnsys SMoraisAnsys commented Apr 19, 2024 •
edited
Loading

Some repos are using an extra step in the documentation build to add download assets to the HTML documentation.
See for example assets page where the assets are listed and a button has been added to download PDF files from HTML documentation.

This PR adds extra steps to zip / copy / paste files to the expected directory doc/_build/html/_static/assets/download.
Build files are expected to be in doc/_build. The PDF file name is expected to be PROJECT_NAME.pdf where PROJECT_NAME is obtained by looking at the project variable in doc/conf.py file. If the file is not found, then every PDF file created in doc/_build/latex will be copy pasted.

Closes #466

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added the enhancement General improvements to existing features label Apr 19, 2024
@RobPasMue
Copy link
Member

Isn't there a way to read the BUILD_DIR variable? That way we don't hardcode it to doc/_build

@SMoraisAnsys
Copy link
Contributor Author

Isn't there a way to read the BUILD_DIR variable? That way we don't hardcode it to doc/_build

We could parse the make.bat or Makefile but if one uses another variable name in the script file, the problem would remain.
Though, as we share the "same" pattern for doc scripts, we could try to retrieve that information and direct to doc/_build if BUILD_DIR is not defined.

@RobPasMue
Copy link
Member

Ah, no need. I see we are uploading artifacts from a hardcoded direction doc/_build/html so all good. You can assume that directory exists

@RobPasMue RobPasMue mentioned this pull request May 30, 2024
Closed
@RobPasMue
Copy link
Member

@SMoraisAnsys - do we have any expected date for this?

SMoraisAnsys and others added 2 commits May 31, 2024 15:39
@SMoraisAnsys
WIP: Clean up
65bb4e8 
Changes include:
- leveraging conf.py to determine PDF filename
- using 'shared' python script to sanitize PDF filename
@SMoraisAnsys
Merge branch 'main' into feat/doc-build-upload-assets
ffb3326
SMoraisAnsys
SMoraisAnsys commented Jun 3, 2024
View reviewed changes
Copy link
Contributor Author

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

To be performed once the action is tested and ready

doc-build/action.yml Outdated Show resolved Hide resolved
doc-build/action.yml Outdated Show resolved Hide resolved
@SMoraisAnsys SMoraisAnsys marked this pull request as ready for review June 3, 2024 12:25
@SMoraisAnsys SMoraisAnsys requested a review from a team as a code owner June 3, 2024 12:25
@SMoraisAnsys
Copy link
Contributor Author

Changes have been tested on pyedb, see https://github.com/ansys/pyedb/actions/runs/8968715375 (note that this run was executed before the last commit which makes this action point to the main branch instead of feat/doc-build-upload-assets)

@SMoraisAnsys
Copy link
Contributor Author

@SMoraisAnsys - do we have any expected date for this?

@RobPasMue Feel free to have a look. The action changed a bit and doesn't leverage the pyproject.toml file anymore (because documentation project don't have a pyproject.toml file). Instead we parse the doc/conf.py file to retrieve the value of project and sanitize it.

@RobPasMue
Copy link
Member

Let me check whenever I get the chance :)

RobPasMue
RobPasMue approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

_doc-build-linux/action.yml Outdated Show resolved Hide resolved
_doc-build-windows/action.yml Outdated Show resolved Hide resolved
@SMoraisAnsys @RobPasMue
Apply suggestions from code review
829cf48 
Co-authored-by: Roberto Pastor Muela <[email protected]>
@RobPasMue
Copy link
Member

Is it ready to merge?

@SMoraisAnsys
Copy link
Contributor Author

SMoraisAnsys commented Jun 5, 2024 •
edited
Loading

Is it ready to merge?

Yep, didn't wanted to merge it myself to let others have a look. I tested it on pyedb and it worked fine for windows and linux.
If you want to have a look, see ansys/pyedb#267

@RobPasMue
Copy link
Member

looks like it works fine. Let's merge and release then

@RobPasMue
Copy link
Member

Unless... @ansys/pyansys-core - any complaints?

clatapie
clatapie approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@clatapie clatapie left a comment

Choose a reason for hiding this comment

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

It looks good to me as well!

Revathyvenugopal162
Revathyvenugopal162 approved these changes Jun 5, 2024
View reviewed changes
_doc-build-linux/action.yml Outdated Show resolved Hide resolved
@SMoraisAnsys @Revathyvenugopal162
Update _doc-build-linux/action.yml
15bc888 
Co-authored-by: Revathy Venugopal <[email protected]>
@SMoraisAnsys SMoraisAnsys mentioned this pull request Jun 6, 2024
Open
8 tasks
@RobPasMue
Copy link
Member

Can we get this merged and released?

@SMoraisAnsys
Copy link
Contributor Author

Can we get this merged and released?

I'm waiting for this feature as much as you haha !

@RobPasMue
Copy link
Member

What's missing??

@SMoraisAnsys
Copy link
Contributor Author

What's missing??

Nothing, I just though that I wasn't the one that should merge the PR.

@SMoraisAnsys SMoraisAnsys merged commit b0d8045 into main Jun 11, 2024
17 checks passed
@SMoraisAnsys SMoraisAnsys deleted the feat/doc-build-upload-assets branch June 11, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxJPRey MaxJPRey MaxJPRey left review comments

@RobPasMue RobPasMue RobPasMue approved these changes

@Revathyvenugopal162 Revathyvenugopal162 Revathyvenugopal162 approved these changes

@clatapie clatapie clatapie approved these changes

Assignees
No one assigned
Labels
enhancement General improvements to existing features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend doc-build action with asset upload
5 participants
@SMoraisAnsys @RobPasMue @clatapie @MaxJPRey @Revathyvenugopal162