Repository navigation
Honor --skip-hooks when given after a subcommand - #1981
Open
tylerbrowndev wants to merge 1 commit into
Open
tylerbrowndev wants to merge 1 commit into
tylerbrowndev wants to merge 1 commit into
Conversation
`kamal app exec --skip-hooks ...` still ran the pre-connect hook. Class options given after a subcommand only reach the subcommand's class options (the case options_with_subcommand_class_options was added for), but run_hook and pre_connect_if_required read options[:skip_hooks], which stays false there. The tests call Kamal::Cli::App directly, where the flag lands in options, so they didn't catch it. Check skip_hooks through options_with_subcommand_class_options, and test exec through Main the way the CLI invokes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approved
No active review issues were identified for the fix to honor --skip-hooks after subcommands.
No open issues 🎉
What changed in this PR
Fixes --skip-hooks handling after a subcommand so commands such as kamal app exec --skip-hooks correctly suppress hooks, including pre-connect.
Changes:
- Adds a shared
skip_hooks?helper that reads merged subcommand class options and is used by both hook checks. - Adds regression tests through
Kamal::Cli::Mainto verify thatpre-connectruns normally and is skipped when requested.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
test/cli/app_test.rb |
Tests hook behavior through the main CLI entry point. |
lib/kamal/cli/base.rb |
Uses merged subcommand options when deciding whether to skip hooks. |
Review effort: Balanced
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
kamal app exec --skip-hooks ...still runs thepre-connecthook.#1754 (for #1752) added
unless options[:skip_hooks]topre_connect_if_required. That covers commands onMain(kamal deploy --skip-hooksworks), but when a class option is given after a subcommand, Thor only puts it in the subcommand's class options.options[:skip_hooks]staysfalsethere, both inpre_connect_if_requiredand inrun_hook.options_with_subcommand_class_optionswas added for exactly this case (1f83b5f), but the hook checks don't use it.The existing exec tests call
Kamal::Cli::Appdirectly, where the flag does land inoptions, so they pass either way.Reproduction (2.12.0 and main)
With
execstubbed to print what it sees:options[:skip_hooks]options_with_subcommand_class_options[:skip_hooks]kamal deploy --skip-hooks/-Htruekamal app exec --skip-hooks ...(any position) /-HfalsetrueWe hit this from a
pre-deployhook that runs a nestedkamal app exec --skip-hooks -d production --roles cron ...: the nested Kamal ranpre-connectagain.Change
skip_hooks?reads the flag throughoptions_with_subcommand_class_options;run_hookandpre_connect_if_requireduse it.test/cli/app_test.rbrunexecthroughKamal::Cli::Main, the way the CLI invokes it: without the flagpre-connectruns, and with--skip-hooksit doesn't. The second fails on main.bin/testlocally: the new tests pass and the run has the same failures as main (the Docker integration tests and the builder tests that need a Docker daemon, which this machine doesn't have).🤖 Generated with Claude Code