-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat: doc deployment #510
feat: doc deployment #510
Conversation
Reviewer's Guide by SourceryThis pull request implements automatic deployment of Sphinx documentation to GitHub Pages when code is pushed to the master branch. It leverages GitHub Actions to build and deploy the documentation. A new GitHub workflow file Sequence diagram for documentation deployment workflowsequenceDiagram
participant D as Developer
participant GH as GitHub
participant GA as GitHub Actions
participant GP as GitHub Pages
D->>GH: Push code to master branch
GH->>GA: Trigger workflow
GA->>GA: Check out repository
GA->>GA: Set up Python 3.11
GA->>GA: Install system packages
GA->>GA: Install Python dependencies
GA->>GA: Build Sphinx documentation
GA->>GP: Deploy HTML files
GP->>GP: Update documentation site
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @r3yc0n1c - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
ubuntu-latest
instead ofubuntu-20.04
to ensure you get security updates and newer package versions unless there's a specific reason for pinning the Ubuntu version. - The version constraints on Jinja2 (<3.1) and Sphinx (5.0.*) seem restrictive. If there's a specific reason for using these older versions, please document it in the PR description. Otherwise, consider using more flexible version constraints.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
@r3yc0n1c Please make a PR against the development branch. At this point of time it also makes more sense to trigger the deployment of the pages when merging into the dev branch.
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 besides the CNAME being unclear
@@ -0,0 +1 @@ | |||
etickets.eventyay.com |
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.
@mariobehling is this the name we want to use? etickets...
?
Why this PR wants to merge to "master"? |
done! |
@r3yc0n1c Still making PR against master branch. |
i thought we needed both... the new pr focuses on the development branch only... we can close this one if not required |
fixes #493
Screenshot
Summary
ImportError: cannot import name 'environmentfilter' from 'jinja2'
(solved by usingJinja2<3.1
)Sphinx version error: The sphinxcontrib.applehelp extension used by this project needs at least Sphinx v5.0;
(solved by usingsphinx==5.0.*
)Summary by Sourcery
Set up Sphinx documentation deployment to GitHub Pages on pushes to the master branch.
CI:
Deployment: