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

Run hooks in parallel on each path #54

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

mcdonnnj
Copy link
Member

🗣 Description

This pull request changes both the packer_fmt and packer_validate hooks to run against each provided path in parallel using command blocks and the & operator. It also removes the custom output for the packer_fmt hook because packer fmt does not return a non-zero exit code if it formats files. The output for the packer_validate hook is updated to correctly work for each path that is provided.

💭 Motivation and context

This will help performance in more complex scenarios such as monorepos with a number of templates and/or files in a template. It also fixes the packer_validate hook to correctly provide output for every path it is run against if any one or more of those paths were to fail validation. Incidentally this obviates the need for #53 because running core hook functionality in command blocks results in a forked process that does not change the current directory of the parent hook process.

🧪 Testing

Automated tests pass. I tested this against scenarios that included a single template and multiple templates to ensure expected output and failure cases.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

Since the `packer fmt` command does not return a non-zero exit code if
file are modified and the `packer validate` command has its own output
if a template fails validation, it does not make sense to add more to
the output of either command. This is especially true since the `packer
fmt` command will output modified files and the `packer validate`
command will mention the failing files in its output.
Wrap the core commands for each hook in command blocks and use the &
operator to run them in the background (through forking). If any
command fails the hook will exit with a non-zero exit code.
Break out the `packer validate` call so we can:
- Disable errexit for _just_ the `packer validate` call so every path
  has an attempt at validation.
- Get the output of the `packer validate` call so that if there is a
  non-zero exit code we can output that along with the path being
  validated.
@mcdonnnj mcdonnnj added bug This issue or pull request addresses broken functionality improvement This issue or pull request will add or improve functionality, maintainability, or ease of use labels Dec 17, 2024
@mcdonnnj mcdonnnj self-assigned this Dec 17, 2024
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Looks great to me! 👍 👍

@mcdonnnj mcdonnnj requested a review from a team December 17, 2024 15:48
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Approved, but please note my one question.

hooks/packer_fmt.sh Show resolved Hide resolved
@mcdonnnj mcdonnnj merged commit 15a2656 into develop Dec 17, 2024
4 checks passed
@mcdonnnj mcdonnnj deleted the improvement/bash_fork_packer_commands branch December 17, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants