Skip to content

Conversation

@reakaleek
Copy link
Member

@reakaleek reakaleek commented Jan 29, 2025

Context

The current script relies on fetch-depth: 0. This causes a checkout time of ~5 minutes on kibana PRs.

Changes

@reakaleek reakaleek added the automation packaging, ci/cd. label Jan 29, 2025
@reakaleek reakaleek requested a review from a team January 29, 2025 09:14
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 }}
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ref: ${{ github.event.pull_request.head.sha }}
# https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
ref: ${{ github.event.pull_request.head.sha }}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@Mpdreamz Mpdreamz left a 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.

@reakaleek reakaleek enabled auto-merge (squash) January 29, 2025 09:51
@reakaleek reakaleek merged commit ced7858 into main Jan 29, 2025
5 checks passed
@reakaleek reakaleek deleted the feature/optimize-asciidoc-comment-workflow branch January 29, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automation packaging, ci/cd.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants