-
Notifications
You must be signed in to change notification settings - Fork 10
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
Optimize comment-on-asciidoc-changes.yml #367
Conversation
fetch-depth: 0 # This is important to fetch all history | ||
# This is considered a security risk when used in conjunction with pull_request_target | ||
# However, we are not running any code from the PR, so it's safe | ||
ref: ${{ github.event.pull_request.head.sha }} |
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.
Maybe add a link on why its a security risk: https://github.com/actions/checkout?tab=readme-ov-file#usage does not mention this :)
fetch-depth: 1 is suppose to work to according to the changed-files github actions docs?
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.
ref: ${{ github.event.pull_request.head.sha }} | |
# https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ | |
ref: ${{ github.event.pull_request.head.sha }} |
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.
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.
fetch-depth: 1 is suppose to work to according to the changed-files github actions docs?
Yes, the examples in https://github.com/tj-actions/changed-files for pull_request
always show the checkout action without any additional inputs.
And as stated in the usage, it's only necessary for the push
event trigger.
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.
I was able to test it here: https://github.com/elastic/docs-eng-playground/pull/17
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.
Some nits and a question but nothing blocking.
Context
The current script relies on
fetch-depth: 0
. This causes a checkout time of ~5 minutes on kibana PRs.Changes
fetch-depth: 0
is only needed for thepush
event and thus not needed for thepull_request_event