Skip to content

Fix reading config file when pull request from forked repository#80

Open
nowsprinting wants to merge 4 commits into
TimonVS:mainfrom
nowsprinting:fix/config_read_in_public_fork
Open

Fix reading config file when pull request from forked repository#80
nowsprinting wants to merge 4 commits into
TimonVS:mainfrom
nowsprinting:fix/config_read_in_public_fork

Conversation

@nowsprinting
Copy link
Copy Markdown

Changes

  • Using base.ref in read config file when pull request from forked repo
  • Add test
  • Add logging

Reason

  • When pull request from forked repo, head.ref is fork-repo's ref. It does not exist in base-repo.

refs #25

@nowsprinting nowsprinting mentioned this pull request Apr 10, 2023
1 task
@nowsprinting nowsprinting changed the title Read config from base.ref when PR from forked repo Fix reading config file when pull request from forked repository Apr 10, 2023
@nowsprinting
Copy link
Copy Markdown
Author

Inner pull request example (regression test)
nowsprinting/pr-labeler-action-test#4

Forked pull request example (fixed this issue, read config from base.ref)
nowsprinting/pr-labeler-action-test#5

@TimonVS
Copy link
Copy Markdown
Owner

TimonVS commented Aug 24, 2023

Thanks for the contributions and a special thank you for including test results 🙌

I suppose this is necessary because it doesn't have permissions to read the fork's config file when using the pull_request_target event?

@nowsprinting
Copy link
Copy Markdown
Author

nowsprinting commented Aug 31, 2023

I suppose this is necessary because it doesn't have permissions to read the fork's config file when using the pull_request_target event?

No. This problem (refs #25 ) has two steps.

  1. Readonly GITHUB_TOKEN and not read secret on pull_request trigger
    So pr-labeler-action is not work. (See: Forked pull request example (pull_request trigger) nowsprinting/pr-labeler-action-test#6 )
    Require using pull_request_target trigger.

  2. fork-repo's head.ref is not exist in base-repo
    Using base.ref.
    Fixed it in this PR.

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.

3 participants