Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 9
fix(attest jira): accept --jira-project-key lists written with spaces#1118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+148
−3
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
4424e29
fix(attest jira): accept `--jira-project-key` lists written with spaces
mbevc1 d7594dc
fix(attest jira): name the offending key, and normalise inside valida…
mbevc1 a29e537
docs(attest jira): say that --jira-project-key takes a comma-separate…
mbevc1 71a392c
chore: amend testing comment to match the actual code
mbevc1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 { | ||
mbevc1 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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 | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
mbevc1 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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", | ||
| }, | ||
| }, | ||
mbevc1 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| { | ||
| 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: [1ABAB-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) { | ||
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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.