Repository navigation
Conversation
cliAutomation
added
the
external
|
Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message. |
|
|
||
| authOk := cmdutil.CheckAuth(factoryConfig) | ||
| if !authOk { | ||
| return fmt.Errorf("Please supply a bundle or authenticate to gh") |
There was a problem hiding this comment.
I thought about using pkg/cmd/root/help.go:authHelp() here but, (1) importing root here would create a circular dependency and (2) it's a private function. I'm open to feedback here!
There was a problem hiding this comment.
Realistically if we go down this route I'd imagine that we have something like cmderrs package used like cmderrs.AuthRequired or something like that.
The main change is previously we always instantiated a TUF client for the public good and GitHub Sigstore instances. Now we only instantiate the TUF client we need, or no client if we are provided a custom trusted root. Note that `gh attestation verify` still requires authentication, that is being addressed in cli#8995. Some other changes are coming along for the ride: - Set TUF cache validity to 1 day, to help serial verification - Attempt to infer verification policy based on custom trusted root - Make command output more friendly if you leave off required arguments Signed-off-by: Zach Steindler <[email protected]>
|
@steiza @williammartin : Wanting to compare this with a generic solution, I believe #9000 could satisfy in a more generic manner. Outside of writing tests as the auth checking logic doesn't have any, I tried reproducing the user workflow using local build. |
|
Closing in favour of #9000, thanks! |
Running the skill on cli/cli#1 printed "- Review decision: " with nothing after it. gh returns "" - not null - for a PR whose decision was never set, and jq's `//` substitutes only for null and false, so the `// "none"` fallback never fired. The same hole sat behind every `.author.login // "unknown"`, where a deleted account's login is likewise "". Reviewing that fix surfaced a worse one in a line it touched: reviews rendered their first body line via `split("\n")[0]`, and `split("\n")` on an empty string returns [] rather than [""], so a bodyless review printed the literal text "null". Every approval left without a comment hit it - reproduced on cli/cli#8995, where both reviews read "(COMMENTED): null". Both traps now have one named jq helper each, defined once and shared by every query rather than restated per call site. Empty-string values print their fallback; a bodyless review or comment reads "(no body)". The statusCheckRollup query keeps its inline test, since its fallback is a chain of sibling fields rather than a literal. Also: one changed file now reads "across 1 file". --json output is byte-identical to before, so pr-review-md and any other JSON consumer are unaffected. Verified against cli/cli#1 (unset decision, single file), #9000 (real decision, multi-line bodies, inline comments) and #8995 (bodyless reviews), plus --json, --diff, --help and the unknown-flag path.
4 participants
If the user does not supply a bundle, then we call a GitHub REST API to fetch the bundle. However, if the user does supply a bundle,
gh attestation verifydoesn't make any GitHub REST API calls, and so the user does not need to be authenticated.