Uh oh!
There was an error while loading. Please reload this page.
fix(attest jira): accept --jira-project-key lists written with spaces - #1118
fix(attest jira): accept --jira-project-key lists written with spaces#1118mbevc1 wants to merge 4 commits into
--jira-project-key lists written with spaces#1118Conversation
Claude finished @mbevc1's task in 3m 20s —— View job Review — round 5 ( |
| Check | Result |
|---|---|
go build ./... | pass |
go vet ./... | pass |
go mod tidy + git diff --exit-code go.mod go.sum | clean |
I could not run go test in this environment (command not permitted here, and the suite needs the local Kosli server plus KOSLI_JIRA_API_TOKEN/KOSLI_JIRA_USERNAME), so the assessment below is by inspection. If you want the test run included in future passes, go test ./cmd/kosli/... needs to be in --allowedTools.
Previous rounds
All three earlier threads are resolved in the current head:
- Ordering hazard — the trim now happens as the first statement of
validateJiraProjectKeys(attestJira.go:408), so a future caller can't reach the regex with untrimmed keys. This was the substantive one. %vhiding an empty key —%qatattestJira.go:417makes[""]visible, with golden and unit tests updated to match.- Validation-only tests — 20b/20c now carry
--assert, so they run the whole path rather than stopping at validation.
This round
Nothing blocking. Two notes:
attestJira_test.go:279-281— the 20c comment still describes a stateinternal/jiracan't produce. Inline suggestion posted; purely cosmetic.- Adjacent, out of scope —
--jira-issue-fieldson this same command is a plainStringVardocumented as "comma separated list" and passed verbatim into the Jirafieldsquery param (attestJira.go:261→internal/jira/jira.go:93).--jira-issue-fields "summary, description"has the same "written the way people type it" shape this PR fixes for project keys, one layer further out. Worth a follow-up check rather than anything here — and it's a raw string, so it isn't covered by the 165-flagnonEmptyValueaudit the PR body defers.
What's good
- Trimming in place means one canonical key reaches both the validator and
FindJiraIssueKeys, which is the right fix — the alternative (each side trimming separately) is exactly the kind of agreement that rots. - The doc comments state why rather than what, including why
%qover%v, so the empty-key rendering can't be silently reverted by someone tidying up verbs. TestValidateJiraProjectKeys's "a padded key is accepted and left trimmed" asserts ono.projectKeysdirectly, which is what actually pins the canonicalisation property — the suite tests are end-to-end colour on top of it.- Deliberately leaving the trailing-comma and whitespace-only-element cases to the existing
nonEmptyValuerule, and saying so in the PR body, keeps the blast radius at one flag. - Help text (
root.go:168) updated to match the newly-supported input;client_referenceis generated at release time so there's nothing checked in to regenerate.
· branch20260820_jira_normalise
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
88c3a85 to
0daa49dCompareUh oh!
There was an error while loading. Please reload this page.
`--jira-project-key` is a `StringSliceVar`, and `pflag` splits those with
encoding/csv, which never trims. So a list written the way most people type one
arrived with the whitespace attached:
`--jira-project-key "ABC, DEF" -> {"ABC", " DEF"}`
validateJiraProjectKeys then rejected `" DEF"`, because a leading space fails
`^[A-Za-z][A-Za-z0-9_]{1,9}$`, and the command exited non-zero on input a user
reasonably expects to work. Verified against a binary built before this change:
`--jira-project-key "ABC, DEF" Error: invalid Jira project keys: [ DEF]`
`--jira-project-key " ABC" Error: invalid Jira project keys: [ ABC]`
Trim the keys in place before validating, so one canonical key reaches both the
validator and `jira.FindJiraIssueKeys` instead of each trimming separately and
having to agree. internal/jira already trims the keys it interpolates; that now
becomes belt-and-braces rather than the only thing standing between a padded key
and a pattern matching `" ABC-123"`.
Two neighbouring behaviours are deliberately left alone. A trailing comma, as in
`--jira-project-key "ABC,"`, is already refused while pflag is still parsing, by
the empty-element rule in `nonEmptyValue.go`, with clearer wording than this
validator could give it. A whitespace-only element slips past that rule, since it
is not empty, so `"ABC, "` still fails here - now displayed as an empty key rather
than a space. Teaching the repo-wide empty-value rule to treat whitespace as
empty would change all 165 flags and wants its own audit.
Closes#1115…tion Review of the whitespace fix, three points. validateJiraProjectKeys now normalises the keys itself instead of relying on run() to have called normaliseJiraProjectKeys first. Nothing enforced that ordering, so a future caller reaching the validator directly got the untrimmed keys rejected again. Invalid keys are reported with %q rather than %v. A key that is only whitespace normalises to "", which %v renders as nothing at all, so --jira-project-key "ABC, " reported `invalid Jira project keys: []` - an error naming no key and reading as though nothing were wrong. It now reads `invalid Jira project keys: [""]`. Test 21's golden is quoted to match; it is the only assertion on this message in the suite. The two CLI cases gain --assert. Without it they only proved validation passed: the success line is printed whether or not any issue matched, so neither showed the trimmed key reaching the matcher. 20b's list is also reordered to "ABC, EX", putting the padded key in the position that has to match the EX-1 commit - as "EX, ABC" the clean first alternative matched either way and --assert would have proved nothing. Note that these cases cannot attribute a failure to this fix alone: jira.MakeJiraIssueKeyPattern trims its keys independently, so an untrimmed key would still match; what fails without this fix is validation, before matching is reached.
…d list
The flag help said only "Can be repeated", so the input shape this branch
exists to fix - --jira-project-key "ABC, DEF" - was not documented anywhere.
Every other list flag in root.go spells it out ("The comma-separated list
of ..."), so this one was the outlier.
The published CLI reference is generated from this string, and client_reference/
is gitignored rather than committed, so the string is the whole docs change with
nothing to regenerate here. Confirmed with make docs CMD="attest jira" that the
generated page picks it up.ef00518 to
a29e537Compare| // --assert so this covers the whole path rather than validation alone: the | ||
| // success line above is printed either way, but a padded key that reached the | ||
| // matcher unusable would find no references and fail the assert. |
There was a problem hiding this comment.
Non-blocking wording nit, and the last thread I have open from the previous round. "a padded key that reached the matcher unusable would find no references" is hedged enough to be true, but it points at a state the code can't reach — internal/jira/jira.go:153 trims before interpolating, so a padded key reaching the matcher is still usable. What --assert actually buys here is that the run goes end to end and EX-1 has to resolve in the real Jira, which is what test 23 establishes.
| // --assert so this covers the whole path rather than validation alone: the | |
| // success line above is printed either way, but a padded key that reached the | |
| // matcher unusable would find no references and fail the assert. | |
| // --assert so this covers the whole path rather than validation alone: without | |
| // the trim the command fails validation and never reaches the matcher, and with | |
| // it the EX-1 commit has to actually resolve in Jira for the case to pass. |
--jira-project-keyis aStringSliceVar, andpflagsplits those with encoding/csv, which never trims. So a list written the way most people type one arrived with the whitespace attached:validateJiraProjectKeys then rejected
" DEF", because a leading space fails^[A-Za-z][A-Za-z0-9_]{1,9}$, and the command exited non-zero on input a user reasonably expects to work. Verified against a binary built before this change:Trim the keys in place before validating, so one canonical key reaches both the validator and
jira.FindJiraIssueKeysinstead of each trimming separately and having to agree. internal/jira already trims the keys it interpolates; that now becomes belt-and-braces rather than the only thing standing between a padded key and a pattern matching" ABC-123".Two neighbouring behaviours are deliberately left alone. A trailing comma, as in
--jira-project-key "ABC,", is already refused while pflag is still parsing, by the empty-element rule innonEmptyValue.go, with clearer wording than this validator could give it. A whitespace-only element slips past that rule, since it is not empty, so"ABC, "still fails here - now displayed as an empty key rather than a space. Teaching the repo-wide empty-value rule to treat whitespace as empty would change all 165 flags and wants its own audit.Closes#1115
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR