Repository navigation
Conversation
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Contributor
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
ivany-nv
reviewed
Oct 16, 2026
ivany-nv
left a comment
Contributor
There was a problem hiding this comment.
Legend: 🚫 BLOCKER = merge-blocking | 💡 SUGGESTION = meaningful improvement | 🧹 NIT = precise small correction
| Finding | |
|---|---|
| 🚫 | .github/scripts/docs_preview.py:79-89 - commits/{sha}/pulls returns nothing for open fork PRs: none of the 13 open fork PRs came back (all 10 same-repo PRs sampled did), so select_preview() returns None for docs-changing forks like #1252 and #1253 whose docs builds succeeded at their current head. Fork PRs therefore never auto-publish, and the greeting comment that pointed their authors at /preview-docs is deleted. Look the PR up with pulls?state=open&head=<head_repository.owner.login>:<head_branch> instead and keep the matches() checks; with that change alone, both PRs are selected correctly. The tests miss this because the fake API at test_docs_preview.py:62-64 answers the commit lookup. |
| 🚫 | .github/workflows/docs-preview.yaml:138-139 - gh api refuses --slurp combined with --jq (gh v2.102.0: "the --slurp option is not supported with --jq or --template"; older gh has no --slurp), so under set -e this step exits right after Deploy preview to gh-pages has pushed. Every successful preview then gets no PR comment and no summary line, the run ends red, and a /preview-docs gets 👎 for a preview that did deploy. Pipe the slurped output into standalone jq -r instead, as the recheck step at L108 already does; #1273 used --paginate --jq without --slurp. |
| 💡 | .github/scripts/docs_preview.py:101-106 - PRs that change only the web client lose their preview. The automatic path requires a docs/ file and both paths require a docs-html artifact (L116-119), which a client-only build never produces, so /preview-docs on such a PR does nothing and leaves no reaction. On main, same-repo client-only PRs publish preview/pr-<N>/client/ (base docs.yaml:256-260), and #1063 and #1148 have live previews of exactly that kind. Accept a lone webapp artifact (at least for same-repo PRs and /preview-docs, making the index.html check at docs-preview.yaml:92 conditional), or say in the PR body that web-client previews are being dropped. |
| 💡 | docs/source/development/contributing.rst:43-73 - The contributor guide still describes the old flow: the same-repo preview URL in the Build & deploy docs Summary tab, a bot comment when a fork PR opens, and fork previews needing a maintainer's /preview-docs. This PR removes all three, so update the section to cover automatic publication for docs-changing PRs, the single updated preview comment, and /preview-docs as a maintainer retry. |
| 🧹 | none |
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Collaborator
Author
|
@ivany-nv Addressed all four findings in 73d47d0:
Validation: 13 tests, full pre-commit, docs build, and artifact staging for docs-only/webapp-only/combined outputs passed. Actionlint passes with the existing unsupported |
Collaborator
Author
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fork PRs currently require
/preview-docsto publish built documentation. Automatically publish previews for PRs changingdocs/, with one updated preview-link comment.The publisher runs trusted default-branch code. Production publishing, maintainer retries, cleanup and GitHub’s fork-build approvals remain unchanged. Same-repository web-client previews remain automatic; maintainers can publish fork web-client previews with
/preview-docs.Validation: 13 tests, full pre-commit, shell checks and read-only GitHub metadata checks passed. Actionlint passes excluding its unsupported existing
concurrency.queuekey. Live publication requires merging intomain. Includes #1273’s notification behavior.