-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
CI: Trigger preview of the web/docs #50899
Changes from all commits
3f65497
5060069
5f8d199
56a2fe5
558488f
9de033b
a90441b
335e202
7149ac5
5e3418b
51683d8
acbd029
89d5dac
d9557d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,3 +89,10 @@ jobs: | |
name: website | ||
path: web/build | ||
retention-days: 14 | ||
|
||
- name: Trigger web/doc preview | ||
run: curl -X POST https://pandas.pydata.org/preview/submit/$RUN_ID/$PR_ID/ | ||
env: | ||
RUN_ID: ${{ github.run_id }} | ||
PR_ID: ${{ github.event.pull_request.number }} | ||
if: github.event_name == 'pull_request' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, this would run every time the PR is pushed to, so we might run out of disk space on the VM hosting our docs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review, and for finding the pending renamed from copying the assign job. I addressed them.
Yes, you're correct. We overwrite the docs, so even if it's updated at every push, we only need one version per PR. But we'll eventually run out of space, since no purging is yet implemented. My idea is to get this working and see how it works for a bit, and if there are no surprises, and this is useful and convenient, I'll continue the pending things. There are some other things pending besides purging, like stop using my personal token, set up the service with systemd, improve the logging. Nothing big, but I think it makes more sense to start using the current MVP first, just in case any major change is needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for clarifying. As long as the service doesn't run on the main docs VM, this is fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are independent. I don't think even the CI will fail if the disk becomes full. But as said, planning to implement the purging and anything else needeed before that happens, once we see everythinf is working fine. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
name: Preview docs | ||
on: | ||
issue_comment: | ||
types: created | ||
|
||
permissions: | ||
contents: read | ||
|
||
jobs: | ||
preview_docs: | ||
permissions: | ||
issues: write | ||
pull-requests: write | ||
runs-on: ubuntu-22.04 | ||
steps: | ||
- if: github.event.comment.body == '/preview' | ||
run: | | ||
if curl --output /dev/null --silent --head --fail "https://pandas.pydata.org/preview/${{ github.event.issue.number }}/"; then | ||
curl -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" -d '{"body": "Website preview of this PR available at: https://pandas.pydata.org/preview/${{ github.event.issue.number }}/"}' https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.issue.number }}/comments | ||
else | ||
curl -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" -d '{"body": "No preview found for PR #${{ github.event.issue.number }}. Did the docs build complete?"}' https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.issue.number }}/comments | ||
fi |
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.
Does this endpoint have any type of rate limiting or session management? Not sure how much we care about a bad actor trying to take it down
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.
Very good point. We're using Cloudflare, hopefully that can help us prevent some problems. The service itself is implemented in asynchronous rust, and it's very efficient, should be able to support quite a big work load. That being said, your comment makes me think of some things that can be improved. Happy to discuss them in private. In any case, I think it's good as it is to start, and if there are attacks, we can consider options, including pausing the service until we can prevent the problem.
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.
Sounds good. As long as you feel like there are other ways to mitigate and react happy to defer to your expertise. Change lgtm overall