Repository navigation
Add pull request viewer previews - #22090
Conversation
Build generic and generic-legacy viewers and report results on the PR. Publish to pdf.js.refs for authors with write access or commits approved with the viewer-preview label. Remove previews on PR closure and prune stale previews during publication.
calixteman
added
To Review
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #22090 +/- ##
=======================================
Coverage 90.28% 90.28%
=======================================
Files 269 269
Lines 67362 67362
=======================================
Hits 60819 60819
Misses 6543 6543
Flags with carried forward coverage won't be shown. Click here to find out more. β View full report in Codecov by Harness. π New features to boost your workflow:
|
timvandermeij
left a comment
timvandermeij
left a comment
There was a problem hiding this comment.
Looks good overall, with a number of comments for consideration.
| - name: Remove the viewers | ||
| env: | ||
| GH_TOKEN: ${{ steps.refs-token.outputs.token }} | ||
| run: bash .github/scripts/viewer-preview/publish-pages.sh |
There was a problem hiding this comment.
Given that the shell scripts already have bash in their shebang, it seems a bit nicer if we could give them the executable bit and then just run them here so that the fact that we invoke bash specifically is only encoded in a single place.
| github.event.action == 'closed' | ||
| runs-on: ubuntu-latest | ||
| environment: | ||
| name: sync_pdfs |
There was a problem hiding this comment.
Is this the correct environment given that we don't actually sync PDF files here? If so, we might want to consider renaming it.
| if: steps.artifact.outputs.found == 'true' | ||
| id: check | ||
| run: | | ||
| set -euo pipefail |
There was a problem hiding this comment.
This is quite a large script, so it would benefit from some small one-line guiding comments for the various stages of validation we perform here to make the flow clearer. Alternatively maybe split it up into a few smaller workflows steps, or even extract the (larger) shell scripts into separate files that we just invoke here (this could make the workflow definitions smaller and thus easier to oversee).
| : "${PREVIEW_REPO:?}" | ||
| : "${PR:?}" | ||
| [[ "$PR" =~ ^[1-9][0-9]*$ ]] || { echo "::error::Invalid PR number: $PR"; exit 1; } | ||
| command -v jq >/dev/null || { echo "::error::jq is required"; exit 1; } |
There was a problem hiding this comment.
Is it necessary to check this? jq is quite common, and in the GitHub Actions context always available AFAIK, and even if it's not the error messages one gets will probably be clear enough anyway.
| done | ||
| } | ||
|
|
||
| for attempt in 1 2 3 4 5; do |
There was a problem hiding this comment.
Using a sequence expression would be a little bit nicer here: for i in {1..5}; do
| if [ -n "$base" ]; then | ||
| prune | ||
| fi | ||
| touch .nojekyll |
There was a problem hiding this comment.
What is this for, since we don't use Jekyll?
timvandermeij
removed
the
To Review
Build generic and generic-legacy viewers and report results on the PR. Publish to pdf.js.refs for authors with write access or commits approved with the viewer-preview label. Remove previews on PR closure and prune stale previews during publication.