Skip to content

Skip Test Live Registry Publish on fork PRs#10997

Open
tehsis wants to merge 1 commit into
masterfrom
pterradillos/skip-test-live-publish-on-fork-prs
Open

Skip Test Live Registry Publish on fork PRs#10997
tehsis wants to merge 1 commit into
masterfrom
pterradillos/skip-test-live-publish-on-fork-prs

Conversation

@tehsis
Copy link
Copy Markdown
Contributor

@tehsis tehsis commented May 13, 2026

Summary

  • Add a fork guard (if: github.event.pull_request.head.repo.full_name == github.repository) to the test-live-publish job in pull-request.yml.
  • Matches the existing pattern already used by sentinel (line 19) and preview (line 165).

The job authenticates to ESC via OIDC, but GitHub does not grant id-token: write to pull_request runs from forks regardless of permissions: write-all. It fails on every fork PR with Unable to get ACTIONS_ID_TOKEN_REQUEST_URL env variable (e.g. #10932). With this guard, the job skips cleanly on fork PRs; same-repo PRs are unaffected.

Trade-off: fork PRs lose the registry-publish dry-run validation. The same check still runs on master via push.yml. If we want the dry-run on fork PRs too, the bigger fix is to refactor push-registry.py --dry-run to skip the authenticated existence-check call so it doesn't need PULUMI_ACCESS_TOKEN.

Test plan

  • Verify same-repo PR still runs Test Live Registry Publish
  • Verify fork PR shows Test Live Registry Publish as skipped (not failed)

🤖 Generated with Claude Code

The job authenticates to ESC via OIDC, but GitHub does not grant
id-token: write to pull_request runs from forks regardless of the
permissions: block. The job fails with "Unable to get
ACTIONS_ID_TOKEN_REQUEST_URL env variable" on every fork PR (e.g. #10932).

Add the same fork guard already used by sentinel and preview so the
job skips cleanly. Same-repo PRs are unaffected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented May 13, 2026

Registry Review — PR #10997

Summary

This is a minimal, correct fix that resolves the consistent failure of Test Live Registry Publish on fork PRs. The root cause analysis in the PR description is accurate: GitHub does not issue an OIDC token to pull_request runs from forks regardless of permissions: write-all, so the ESC OIDC auth step cannot succeed. Guarding the job with if: github.event.pull_request.head.repo.full_name == github.repository is the established pattern in this workflow.

What I checked

  • Pattern consistency: The if: guard matches the existing usage in sentinel (.github/workflows/pull-request.yml:19) and preview (.github/workflows/pull-request.yml:167). The preview job adds an extra label exclusion (automation/tfgen-provider-docs); omitting that here is correct since the dry-run is cheap and there's no equivalent reason to skip on metadata-only PRs.
  • Sentinel dependency: sentinel lists test-live-publish in its needs: (.github/workflows/pull-request.yml:29). On fork PRs, both jobs now skip together (same guard expression), so the needs: relationship doesn't create a broken/perpetually-pending check. ✅
  • Trigger compatibility: The workflow only fires on pull_request (.github/workflows/pull-request.yml:10-12), so github.event.pull_request.head.repo.full_name is always populated — the conditional is safe.
  • YAML: Indentation and key ordering are clean; the if: is placed adjacent to name:/runs-on: consistent with other jobs.

Observations (non-blocking)

  • The PR description references preview at "line 165," but the if: for preview is actually at .github/workflows/pull-request.yml:167 (line 165 is the comment header for the job). Minor; doesn't affect the change.
  • The trade-off called out in the description — fork PRs losing the registry-publish dry-run — is a real but acceptable gap, and push.yml continues to exercise it on master. The follow-up suggestion (refactor push-registry.py --dry-run to skip the authenticated existence check so the job no longer needs PULUMI_ACCESS_TOKEN) is worth filing as a separate issue if someone wants fork-PR coverage back.

Verdict

LGTM. Two-line change, scoped narrowly, matches existing conventions, and the reasoning is captured both in an inline comment and the PR description.


Mention @claude if you'd like additional reviews or fixes.

@github-actions
Copy link
Copy Markdown
Contributor

Your site preview for commit b179396 is ready! 🎉

http://registry--origin-pr-10997-b1793964.s3-website.us-west-2.amazonaws.com/registry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant