oidc-exchange: specialize error on PRs from forks #203
Conversation
oidc-exchange.py
Outdated
| def event_is_third_party_pr() -> bool: | ||
| # Non-`pull_request` events cannot be from third-party PRs. | ||
| if os.getenv("GITHUB_EVENT_NAME") != "pull_request": | ||
| return False | ||
|
|
||
| if event_path := os.getenv("GITHUB_EVENT_PATH"): | ||
| try: | ||
| event = json.loads(Path(event_path).read_text()) | ||
| try: | ||
| return event["pull_request"]["head"]["repo"]["fork"] | ||
| except KeyError: | ||
| return False | ||
| except json.JSONDecodeError: | ||
| debug("unexpected: GITHUB_EVENT_PATH does not contain valid JSON") | ||
| return False | ||
|
|
||
| # No GITHUB_EVENT_PATH indicates a weird GitHub or runner bug. | ||
| debug("unexpected: no GITHUB_EVENT_PATH to check") | ||
| return False |
There was a problem hiding this comment.
NB: I used debug(...) rather than die(...) here, since this function is only called from an error path -- I think masking the error here would likely cause even more confusion. But maybe there's a better idea/solution 🙂
There was a problem hiding this comment.
Thanks for this note! I don't have better a refactoring suggestion for this right now. Maybe sometime...
There was a problem hiding this comment.
@woodruffw love the idea! I only got a few small suggestions.
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
|
@woodruffw I added one last suggestion for a thing that wasn't obvious when I was checking it out from the mobile client. Looking at your commits, I take it you expect me to use the squash strategy, right? |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Thanks! And yeah, that's what I was thinking -- that being said, I can also squash this locally and force-push if that's preferred (including going forwards). |
I usually prefer nice atomic commits and a natural Git merge mode (not squash or rebase) to preserve history. But if that's not observed, I'm okay with selecting a squash mode, as long as it's better and/or requested. If it's not requested, I'll use my best judgement but I still wanted to clarify just in case you have your own preferences. The reason is that the responsibility for writing a good commit message shifts towards the merger in the case of squash compared to merge where the contributor would compose commit individual messages and the maintainer would only be adding something to the merge commit message, not touching the atomic ones. |
This specializes the token retrieval error handling a bit, providing an alternative error message when the error cause is something that we know can't possibly work due to GitHub's own restrictions on PRs from forks.
Closes #202.
See python-pillow/Pillow#7616.