From 48512176d8416c4f046cc49d88b13901acb253b1 Mon Sep 17 00:00:00 2001 From: Chris Westra Date: Wed, 14 Sep 2022 11:57:51 -0400 Subject: [PATCH] Error when issue-repo doesn't match issue url --- pkg/cmd/issue/develop/develop.go | 49 ++++++++++++++++++++------- pkg/cmd/issue/develop/develop_test.go | 26 +++++++++++--- pkg/cmd/issue/shared/lookup.go | 10 ++---- 3 files changed, 62 insertions(+), 23 deletions(-) diff --git a/pkg/cmd/issue/develop/develop.go b/pkg/cmd/issue/develop/develop.go index b6e0119f6..a8e73add0 100644 --- a/pkg/cmd/issue/develop/develop.go +++ b/pkg/cmd/issue/develop/develop.go @@ -60,7 +60,7 @@ func NewCmdDevelop(f *cmdutil.Factory, runF func(*DevelopOptions) error) *cobra. if opts.List { return developRunList(opts) } - return developRun(opts) + return developRunCreate(opts) }, } fl := cmd.Flags() @@ -72,7 +72,7 @@ func NewCmdDevelop(f *cmdutil.Factory, runF func(*DevelopOptions) error) *cobra. return cmd } -func developRun(opts *DevelopOptions) (err error) { +func developRunCreate(opts *DevelopOptions) (err error) { httpClient, err := opts.HttpClient() if err != nil { return err @@ -82,13 +82,14 @@ func developRun(opts *DevelopOptions) (err error) { if err != nil { return err } - opts.IO.StartProgressIndicator() - repo, err := api.GitHubRepo(apiClient, baseRepo) + + issueNumber, issueRepo, err := issueMetadata(opts.IssueSelector, opts.IssueRepoSelector, baseRepo) if err != nil { return err } - issueNumber, issueRepo, err := issueMetadata(opts.IssueSelector, opts.IssueRepoSelector, baseRepo) + opts.IO.StartProgressIndicator() + repo, err := api.GitHubRepo(apiClient, baseRepo) if err != nil { return err } @@ -132,23 +133,47 @@ func developRun(opts *DevelopOptions) (err error) { return } -func issueMetadata(issueSelector string, issueRepoSelector string, baseRepo ghrepo.Interface) (issueNumber int, issueRepo ghrepo.Interface, err error) { +// If the issue is in the base repo, we can use the issue number directly. Otherwise, we need to use the issue's url or the IssueRepoSelector argument. +// If the repo from the URL doesn't match the IssueRepoSelector argument, we error. +func issueMetadata(issueSelector string, issueRepoSelector string, baseRepo ghrepo.Interface) (issueNumber int, issueFlagRepo ghrepo.Interface, err error) { + var targetRepo ghrepo.Interface if issueRepoSelector != "" { - issueRepo, err = ghrepo.FromFullNameWithHost(issueRepoSelector, baseRepo.RepoHost()) + issueFlagRepo, err = ghrepo.FromFullNameWithHost(issueRepoSelector, baseRepo.RepoHost()) if err != nil { return 0, nil, err } } - targetRepo := baseRepo - if issueRepo != nil { - targetRepo = issueRepo + if issueFlagRepo != nil { + targetRepo = issueFlagRepo } - issueNumber, issueRepo, err = shared.IssueNumberAndRepoFromArg(issueSelector, targetRepo) + + issueNumber, issueArgRepo, err := shared.IssueNumberAndRepoFromArg(issueSelector) if err != nil { return 0, nil, err } - return issueNumber, issueRepo, nil + + if issueArgRepo != nil { + targetRepo = issueArgRepo + + if issueFlagRepo != nil { + differentOwner := (issueFlagRepo.RepoOwner() != issueArgRepo.RepoOwner()) + differentName := (issueFlagRepo.RepoName() != issueArgRepo.RepoName()) + if differentOwner || differentName { + return 0, nil, fmt.Errorf("issue repo in url %s/%s does not match the repo from --issue-repo %s/%s", issueArgRepo.RepoOwner(), issueArgRepo.RepoName(), issueFlagRepo.RepoOwner(), issueFlagRepo.RepoName()) + } + } + } + + if issueFlagRepo == nil && issueArgRepo == nil { + targetRepo = baseRepo + } + + if targetRepo == nil { + return 0, nil, fmt.Errorf("could not determine issue repo") + } + + return issueNumber, targetRepo, nil } func developRunList(opts *DevelopOptions) (err error) { diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index 2b8927e0a..fd7296559 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -162,7 +162,7 @@ func Test_developRun(t *testing.T) { }, expectedOut: "foo\nbar\n", }, - {name: "list branches for an issue providing an issue url and specifying its repo returns an error", + {name: "list branches for an issue providing an issue url and specifying the same repo works", setup: func(opts *DevelopOptions, t *testing.T) func() { opts.IssueSelector = "https://github.com/cli/test-repo/issues/42" opts.IssueRepoSelector = "cli/test-repo" @@ -204,7 +204,16 @@ func Test_developRun(t *testing.T) { assert.Equal(t, "test-repo", inputs["repositoryName"]) })) }, - expectedErrOut: "error: --issue-repo cannot be used when providing an issue URL\n", + expectedOut: "foo\nbar\n", + }, + {name: "list branches for an issue providing an issue url and specifying a different repo returns an error", + setup: func(opts *DevelopOptions, t *testing.T) func() { + opts.IssueSelector = "https://github.com/cli/test-repo/issues/42" + opts.IssueRepoSelector = "cli/other" + opts.List = true + return func() {} + }, + wantErr: "issue repo in url cli/test-repo does not match the repo from --issue-repo cli/other", }, {name: "develop new branch", setup: func(opts *DevelopOptions, t *testing.T) func() { @@ -243,6 +252,14 @@ func Test_developRun(t *testing.T) { }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", }, + {name: "develop providing an issue url and specifying a different repo returns an error", + setup: func(opts *DevelopOptions, t *testing.T) func() { + opts.IssueSelector = "https://github.com/cli/test-repo/issues/42" + opts.IssueRepoSelector = "cli/other" + return func() {} + }, + wantErr: "issue repo in url cli/test-repo does not match the repo from --issue-repo cli/other", + }, {name: "develop new branch with checkout when the branch exists locally", setup: func(opts *DevelopOptions, t *testing.T) func() { opts.Name = "my-branch" @@ -288,7 +305,8 @@ func Test_developRun(t *testing.T) { cs.Register(`git pull --ff-only origin my-branch`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", - }, {name: "develop new branch with checkout when the branch does not exist locally", + }, + {name: "develop new branch with checkout when the branch does not exist locally", setup: func(opts *DevelopOptions, t *testing.T) func() { opts.Name = "my-branch" opts.BaseBranch = "main" @@ -398,7 +416,7 @@ func Test_developRun(t *testing.T) { err = developRunList(&opts) } else { - err = developRun(&opts) + err = developRunCreate(&opts) } output := &test.CmdOut{ OutBuf: stdout, diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index 3a4d6594e..abd22e021 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -60,9 +60,9 @@ func issueMetadataFromURL(s string) (int, ghrepo.Interface) { return issueNumber, repo } -//TODO: Returns the issue number and base repo if the issue URL is provided, falling back to the -// supplied repo if the base repo is not specified in the URL. -func IssueNumberAndRepoFromArg(arg string, fallbackRepo ghrepo.Interface) (int, ghrepo.Interface, error) { +// Returns the issue number and repo if the issue URL is provided. +// If only the issue number is provided, returns the number and nil repo. +func IssueNumberAndRepoFromArg(arg string) (int, ghrepo.Interface, error) { issueNumber, baseRepo := issueMetadataFromURL(arg) if issueNumber == 0 { @@ -73,10 +73,6 @@ func IssueNumberAndRepoFromArg(arg string, fallbackRepo ghrepo.Interface) (int, } } - if baseRepo == nil { - baseRepo = fallbackRepo - } - return issueNumber, baseRepo, nil }