feat: pf apply all #294

jlevydev · 2025-03-05T16:58:18Z

When upgrading Panfactum versions and applying the infrastructure the current workflow is to use terragrunt run-all apply in order to run the full dependency graph for an environment. While a good utility in a stable state, this command seems like the wrong tool in the context of an upgrade where there may be breakages. This is because run-all is an all or nothing operation and all the work from previous layers of the dependency graph must be repeated to get to later layers in the case of a failure.

This pull request introduces a custom utility as an alternative pf-apply-all. It let's you do a terragrunt run-all apply in a more interactive way. The script works as follows:

Generate the dependency graph using terragrunt output-module-groups
For each group ask to run the group or skip
On run:
a. Run the whole group in parallel
b. Report all failed module and allow the user to infinitely re-run each failed module interactively one by one
Repeat 2-3 until all the groups are processed
This lets the user debug individual module failures and then continue forward. It also enables starting at an arbitrary depth of the dependency graph.

A consideration for the WIP script is that it creates and then destroys log files in a temp directory at the root using mktemp. Might be worth moving that to a directory in the repo and not deleting them so you have a more complete audit history of the applies that happened in the background. If the logs were retained it would improve visibility vs the previous paradigm where the logs are all comingled and somewhat worthless to the user.

Tested successfully on WSL.

Important

Introduces pf-apply-all.sh for interactive Terragrunt module application with improved failure handling and parallel processing.

Behavior:
- Introduces pf-apply-all.sh for interactive Terragrunt module application.
- Allows retry, skip, or exit on module failures.
- Processes modules in parallel within groups.
Implementation:
- Uses terragrunt output-module-groups to generate dependency graph.
- Creates temporary log files for each module application.
- Deletes log files after processing, with a note on potential improvement for audit history.
Dependencies:
- Requires jq for JSON processing.

^{This description was created by}^{for 170e3c2. It will automatically update as commits are pushed.}

ellipsis-dev

❌ Changes requested. Reviewed everything up to 170e3c2 in 2 minutes and 57 seconds

More details

Looked at 248 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 7 drafted comments based on config settings.

1. packages/nix/packages/scripts/pf-apply-all.sh:82

Draft comment:
Reading user input directly via /dev/tty is good; however, consider validating input strictly in case of unexpected responses.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

2. packages/nix/packages/scripts/pf-apply-all.sh:241

Draft comment:
No newline at the end of file. Consider adding a newline.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While having a newline at the end of files is a common Unix convention, this is a very minor style issue that doesn't affect functionality. The rules state not to make comments that are obvious or unimportant. This seems like a trivial issue that doesn't require a comment.
Missing newlines can sometimes cause issues with certain tools or when concatenating files. It is a widely accepted best practice.
While true, any serious issues would be caught by linters or the build system. This is too minor to warrant a PR comment.
Delete this comment as it points out a trivial style issue that doesn't meaningfully impact the code.

3. packages/nix/packages/scripts/pf-apply-all.sh:99

Draft comment:
Consider adding a trap to clean up the temporary directory in case the script terminates unexpectedly.
Reason this comment was not posted:
Marked as duplicate.

4. packages/nix/packages/scripts/pf-apply-all.sh:51

Draft comment:
Check that the module path exists before attempting to change directory. This could provide clearer error messaging if the path is invalid.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The module paths come from terragrunt itself, so they should be valid. If a path is invalid, the cd will fail and be caught by the error handling. Adding an explicit check doesn't add much value since the error will be caught and handled gracefully anyway. The script already has good error reporting via log files and colored output.
The comment has a point that explicit validation could provide clearer error messages. There's a small chance the paths from terragrunt are stale.
The existing error handling is sufficient - failed modules are clearly reported with logs, and the user can retry or skip them. Adding path validation would be redundant.
The comment should be deleted as the existing error handling is adequate and adding path validation would be redundant.

5. packages/nix/packages/scripts/pf-apply-all.sh:83

Draft comment:
Handle empty user input from /dev/tty. If the user simply sends an EOF (or empty input), the script might behave unexpectedly.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The current code actually handles empty input gracefully - if response is empty, it won't match ^[Ee]$ or ^[Ss]$, so it will default to retrying which is a reasonable behavior. The suggestion would make the empty case more explicit but doesn't change the behavior. The code is already defensive.
I could be underestimating the importance of explicitly handling EOF conditions in shell scripts. There may be edge cases I'm not considering.
While being explicit about EOF handling is good practice, the current code already handles empty input safely by falling through to the retry case. The suggested change doesn't improve functionality.
The comment should be removed since the current code already handles empty input gracefully and the suggestion doesn't improve the behavior.

6. packages/nix/packages/scripts/pf-apply-all.sh:155

Draft comment:
Using separate arrays (pids, module_paths, log_files) to correlate modules can be error-prone. Consider using associative arrays or a single compound data structure.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The suggestion has merit from a code quality perspective - using separate indexed arrays can be error-prone if indices get out of sync. However, in this case the code is relatively straightforward, the arrays are scoped locally, and the indexing is managed consistently. The suggested alternatives (associative arrays) would add complexity without clear benefits.
The comment identifies a real code quality concern. Parallel array usage is generally considered an anti-pattern in software engineering.
While the concern is valid, the current implementation is simple and works well for this specific use case. The arrays are local in scope and the indexing is straightforward.
The comment should be removed as it suggests a refactor that would add complexity without significant benefit in this specific context.

7. packages/nix/packages/scripts/pf-apply-all.sh:241

Draft comment:
Add a newline at the end of the file. This is a minor POSIX best practice.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

Workflow ID: wflow_2SSJkOAuVR296Ufd

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-03-05T17:01:25Z