-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
oidc-exchange: specialize error on PRs from forks #203
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@woodruffw love the idea! I only got a few small suggestions.
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
@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 (Святослав Сидоренко) <[email protected]>
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.