-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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
dav3r
approved these changes
Dec 17, 2024
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.
Looks great to me! 👍 👍
jsf9k
approved these changes
Dec 17, 2024
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.
Approved, but please note my one question.
dv4harr10
approved these changes
Dec 17, 2024
This was referenced Dec 17, 2024
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🗣 Description
This pull request changes both the
packer_fmt
andpacker_validate
hooks to run against each provided path in parallel using command blocks and the&
operator. It also removes the custom output for thepacker_fmt
hook becausepacker fmt
does not return a non-zero exit code if it formats files. The output for thepacker_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