Skip to content
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

Merged
merged 6 commits into from
Jan 29, 2025

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.

2 participants