add prlink to cli output #457
Conversation
pkg/gitproviders/common.go
Outdated
| @@ -260,11 +260,11 @@ func UploadDeployKey(owner, repoName string, deployKey []byte) error { | |||
| func DeployKeyExists(owner, repoName string) (bool, error) { | |||
| return gitProviderHandler.(GitProviderHandler).DeployKeyExists(owner, repoName) | |||
| } | |||
| func CreatePullRequestToUserRepo(userRepRef gitprovider.UserRepositoryRef, targetBranch string, newBranch string, files []gitprovider.CommitFile, commitMessage string, prTitle string, prDescription string) error { | |||
| func CreatePullRequestToUserRepo(userRepRef gitprovider.UserRepositoryRef, targetBranch string, newBranch string, files []gitprovider.CommitFile, commitMessage string, prTitle string, prDescription string) (string, error) { | |||
There was a problem hiding this comment.
There are way too many arguments to this function. Anything more than 4 tends to get unwieldy. I realize it was set up that way before this PR, just wanted to comment.
There was a problem hiding this comment.
Given that this func returns a string, I don't know what the string represents. I assume a URL in this case? Can we return a golang url.Url type? Or possibly the pull request object that is being created?
There was a problem hiding this comment.
I can absolutely return the pr object and get the url string in add. Unfortunately I dont think I can lower the number of arguments. Gitproviders needs all of that info and I could take some of that info and generate it in common but that could limit future functionality.
There was a problem hiding this comment.
Yeah I would say that would be a separate PR to make that change.
I would use a struct as an argument like we do on AddParams to make it a little more understandable.
| provider, err := GithubProvider() | ||
| if err != nil { | ||
| return err | ||
| return nil, err |
There was a problem hiding this comment.
Wrapped error here would help with debugging.
addresses https://app.zenhub.com/workspaces/weaveworks-foss-603cb6b1d4dff9000e242dc4/issues/weaveworks/weave-gitops/374 outputs the pr link to the cli output.
use the cli output to merge the pr in the test