Auto-install hawkeye in ensure-hawkeye-exists.sh#1644
Conversation
|
Hi @harshitsinghbhandari! Thanks for filing the issues and the PRs. Let me look over your worktree hook fix first, and then I'll think a bit more regarding the |
|
Thanks for taking a look! Happy to reshape this PR toward either approach based on what you think makes sense — keeping |
@harshitsinghbhandari This is a good point. I'm not a fan of it myself, and we shouldn't be doing it without their knowledge. What do you think about the following UX:
We'd need to double-check that any non-interactive workflows (I'm thinking mainly about our Git workflows where each build runs on a freshly imaged machine) explicitly install hawkeye non-interactively so that subsequent steps that run the license/format checks don't stall waiting for input. |
|
@jglogan — agreed on all three points. Here's the plan:
CI / GitHub Actions:
Will push the implementation shortly. |
bc7af87 to
3430c40
Compare
What would be downloaded and where, and that what's downloaded will be passwd into |
jglogan
left a comment
There was a problem hiding this comment.
Left one comment about describing to the user what the hawkeye installer does.
3430c40 to
dfaddfc
Compare
`make pre-commit` previously installed the hook but did not check for
hawkeye, so the very next commit attempt would fail. v1 of this change
auto-installed hawkeye via `exec install-hawkeye.sh` when it was
missing, but `curl | sh` without prior user consent is a security
anti-pattern.
This change instead:
- Makes the missing-hawkeye path interactive by default: print exactly
what would be downloaded and from where, then prompt `[y/N]`.
- Supports two ways to skip the prompt non-interactively (matching the
convention used by terraform, kubectl, apt-get, etc.):
- CLI flag: `--auto-install` (or `-y`)
- Env var: HAWKEYE_AUTO_INSTALL=1
Precedence: flag > env var > prompt (default).
- Refuses to install silently when stdin is not a TTY *and* neither
consent mechanism was provided, with a clear error pointing at both
knobs. This protects piped or CI invocations that forgot to opt in.
- Wires `make pre-commit` to run the ensure script after installing
the hook, so contributors hit the consent prompt at setup time rather
than at their first commit.
- Exports HAWKEYE_AUTO_INSTALL=1 in the GitHub Actions `Check
formatting` step so license/format checks don't stall on the prompt.
Fixes apple#1642.
dfaddfc to
d19291d
Compare
|
Applied your suggestion in |
Code Coverage
|
jglogan
left a comment
There was a problem hiding this comment.
@harshitsinghbhandari Looks good, thank you for cleaning this up!
Summary
scripts/ensure-hawkeye-exists.shpreviously checked for hawkeye and exited 1 with a message asking the contributor to install it manually. Sincemake pre-commitdoesn't run the installer either, every fresh contributor hits the same wall on their first commit —make pre-commitsucceeds, but the very next commit attempt fails onmake checkwith no signal from the earlier step that anything was missing.This change makes
ensure-hawkeye-exists.shinstall hawkeye when it's missing, byexec-ing the existingscripts/install-hawkeye.sh. "Ensure" now does what the name suggests.If you'd prefer the install to stay opt-in (e.g. so contributors stay in explicit control of
curl | shflows), I'm happy to convert this to an alternative shape — havingmake pre-commitdepend on a target that installs hawkeye, or havingmake pre-commitprint a clear note that hawkeye must be installed separately.Fixes #1642.
Test plan
.local/bin/hawkeye, runscripts/ensure-hawkeye-exists.sh— installs hawkeye and exits 0.scripts/ensure-hawkeye-exists.shwhen hawkeye is already present — prints "hawkeye found!" and exits 0 (unchanged).make checksucceeds after a single end-to-endmake pre-commit+ commit cycle on a fresh checkout.