diff --git a/cmd/kosli/attestJira.go b/cmd/kosli/attestJira.go index c0c10f19e..b300c57f7 100644 --- a/cmd/kosli/attestJira.go +++ b/cmd/kosli/attestJira.go @@ -388,7 +388,24 @@ func jiraSearchText(commitInfo *gitview.CommitInfo, secondarySource string, igno // But Jira itself will accept lower case letters when searching a repository for matching branches and commits. var jiraProjectKeyRegexp = regexp.MustCompile("^[A-Za-z][A-Za-z0-9_]{1,9}$") +// normaliseJiraProjectKeys trims each project key in place. cobra splits a comma-separated +// list with encoding/csv, which does not trim, so --jira-project-key "ABC, DEF" arrives as +// {"ABC", " DEF"} and the untrimmed fragment fails validation. Normalising means a single +// canonical key reaches both the validation below and jira.FindJiraIssueKeys, rather than +// each trimming separately and having to agree. +func (o *attestJiraOptions) normaliseJiraProjectKeys() { + for i, projectKey := range o.projectKeys { + o.projectKeys[i] = strings.TrimSpace(projectKey) + } +} + +// validateJiraProjectKeys normalises the keys first, so that a caller cannot reach the +// validation without it and get the untrimmed keys rejected. +// +// Keys are reported with %q, because a key that is only whitespace normalises to "" and %v +// renders that as nothing at all, leaving an error naming no key. func (o *attestJiraOptions) validateJiraProjectKeys() error { + o.normaliseJiraProjectKeys() invalidKeys := []string{} for _, projectKey := range o.projectKeys { isValid := jiraProjectKeyRegexp.MatchString(projectKey) @@ -397,7 +414,7 @@ func (o *attestJiraOptions) validateJiraProjectKeys() error { } } if len(invalidKeys) > 0 { - return fmt.Errorf("invalid Jira project keys: %v", invalidKeys) + return fmt.Errorf("invalid Jira project keys: %q", invalidKeys) } return nil } diff --git a/cmd/kosli/attestJira_test.go b/cmd/kosli/attestJira_test.go index 0fd926802..3946bd77d 100644 --- a/cmd/kosli/attestJira_test.go +++ b/cmd/kosli/attestJira_test.go @@ -260,6 +260,36 @@ func (suite *AttestJiraCommandTestSuite) TestAttestJiraCmd() { commitMessage: "low-1 test commit", }, }, + { + // cobra splits the list with encoding/csv, which does not trim, so without + // normalisation the " EX" fragment fails validation and the command never + // reaches the matcher. --assert carries the rest of the path end to end. + name: "20b can specify jira project keys as a comma-separated list with spaces", + cmd: fmt.Sprintf(`attest jira --name bar + --jira-base-url https://kosli-test.atlassian.net + --jira-project-key "ABC, EX" + --repo-root %s + --assert %s`, suite.tmpDir, suite.defaultKosliArguments), + golden: "jira attestation 'bar' is reported to trail: test-123\n", + additionalConfig: jiraTestsAdditionalConfig{ + commitMessage: "EX-1 test commit", + }, + }, + { + // --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. + name: "20c a jira project key padded with spaces is accepted", + cmd: fmt.Sprintf(`attest jira --name bar + --jira-base-url https://kosli-test.atlassian.net + --jira-project-key " EX " + --repo-root %s + --assert %s`, suite.tmpDir, suite.defaultKosliArguments), + golden: "jira attestation 'bar' is reported to trail: test-123\n", + additionalConfig: jiraTestsAdditionalConfig{ + commitMessage: "EX-1 test commit", + }, + }, { wantError: true, name: "21 fails with an invalid Jira project key specified", @@ -268,7 +298,7 @@ func (suite *AttestJiraCommandTestSuite) TestAttestJiraCmd() { --jira-project-key 1AB --jira-project-key AB-44 --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), - golden: "Error: invalid Jira project keys: [1AB AB-44]\n", + golden: "Error: invalid Jira project keys: [\"1AB\" \"AB-44\"]\n", additionalConfig: jiraTestsAdditionalConfig{ commitMessage: "EX-1 test commit", }, @@ -397,6 +427,104 @@ func TestJiraSearchText(t *testing.T) { } } +func TestNormaliseJiraProjectKeys(t *testing.T) { + for _, tc := range []struct { + name string + projectKeys []string + want []string + }{ + { + // cobra splits on the comma without trimming, so this is what + // --jira-project-key "ABC, DEF" actually delivers + name: "a space after the comma is trimmed", + projectKeys: []string{"ABC", " DEF"}, + want: []string{"ABC", "DEF"}, + }, + { + name: "space on both sides is trimmed", + projectKeys: []string{" EX "}, + want: []string{"EX"}, + }, + { + name: "tabs and newlines are trimmed", + projectKeys: []string{"\tEX", "ABC\n"}, + want: []string{"EX", "ABC"}, + }, + { + name: "keys that need no trimming are left alone", + projectKeys: []string{"ABC", "low", "A_99"}, + want: []string{"ABC", "low", "A_99"}, + }, + { + // a trailing comma yields an empty fragment, which stays empty so that + // validateJiraProjectKeys still rejects it + name: "an empty key stays empty", + projectKeys: []string{"ABC", ""}, + want: []string{"ABC", ""}, + }, + { + name: "a whitespace-only key becomes empty and is still rejected downstream", + projectKeys: []string{"ABC", " "}, + want: []string{"ABC", ""}, + }, + { + name: "no keys is left alone", + projectKeys: []string{}, + want: []string{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + o := &attestJiraOptions{projectKeys: tc.projectKeys} + o.normaliseJiraProjectKeys() + require.Equal(t, tc.want, o.projectKeys) + }) + } +} + +// TestValidateJiraProjectKeys pins that validation normalises the keys itself, rather than +// relying on its caller to have done it, and that a key which is only whitespace is named in +// the error instead of rendering as nothing. +func TestValidateJiraProjectKeys(t *testing.T) { + for _, tc := range []struct { + name string + projectKeys []string + wantErr string + wantKeys []string + }{ + { + name: "a padded key is accepted and left trimmed", + projectKeys: []string{" EX "}, + wantKeys: []string{"EX"}, + }, + { + name: "a padded comma-separated list is accepted", + projectKeys: []string{"ABC", " EX"}, + wantKeys: []string{"ABC", "EX"}, + }, + { + name: "an invalid key is still rejected, and quoted", + projectKeys: []string{"1AB", "AB-44"}, + wantErr: `invalid Jira project keys: ["1AB" "AB-44"]`, + }, + { + name: "a whitespace-only key is named in the error rather than rendering as nothing", + projectKeys: []string{"ABC", " "}, + wantErr: `invalid Jira project keys: [""]`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + o := &attestJiraOptions{projectKeys: tc.projectKeys} + err := o.validateJiraProjectKeys() + if tc.wantErr != "" { + require.EqualError(t, err, tc.wantErr) + return + } + require.NoError(t, err) + require.Equal(t, tc.wantKeys, o.projectKeys) + }) + } +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestAttestJiraCommandTestSuite(t *testing.T) { diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index 144650b3d..37d417fcb 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -165,7 +165,7 @@ The ^.kosli_ignore^ will be treated as part of the artifact like any other file, jiraUsernameFlag = "Jira username (for Jira Cloud)" jiraAPITokenFlag = "Jira API token (for Jira Cloud)" jiraPATFlag = "Jira personal access token (for self-hosted Jira)" - jiraProjectKeyFlag = "[optional] Jira project key to match against. Can be repeated. Defaults to matching any jira project key." + jiraProjectKeyFlag = "[optional] Jira project key to match against. Can be repeated, or given as a comma-separated list. Defaults to matching any jira project key." jiraIssueFieldFlag = "[optional] The comma separated list of fields to include from the Jira issue. Default no fields are included. '*all' will give all fields." jiraSecondarySourceFlag = "[optional] An optional string to search for Jira ticket reference, e.g. '--jira-secondary-source ${{ github.head_ref }}'" ignoreBranchMatchFlag = "Ignore branch name when searching for Jira ticket reference."