Conversation
|
@mislav I'd like some insight on how I can simulate a 403 response when the user doesn't have WRITE permission on the organization. I'm stuck at that last test case. Would be helpful if you gave some advice. Thank you :) |
mislav
left a comment
There was a problem hiding this comment.
Thanks for doing this!
I've left code review comments, but I do not expect you to address them. I've been doing cleanups myself as I was reviewing the code and I've opened another PR (as this one doesn't let me push to it) #3611
| path := fmt.Sprintf("repos/%s/forks", ghrepo.FullName(repo)) | ||
| body := bytes.NewBufferString(`{}`) | ||
| if org != "" { | ||
| orgBody := fmt.Sprintf(`{ "organization": "%s" }`, org) |
There was a problem hiding this comment.
Let's use JSON encoding functionality instead of writing it by hand.
| var httpErr HTTPError | ||
| if errors.As(err, &httpErr) { | ||
| if httpErr.StatusCode == http.StatusForbidden { | ||
| return nil, fmt.Errorf("Must have admin rights to Repository") |
There was a problem hiding this comment.
I appreciate you adding this special case, but I think it might be premature optimization. The 401 error will be reported to the terminal via our usual error handling and I don't think we need to add anything on top of that. If the user doesn't have write access to their org, the fork command will fail with a visible 401 and it will be enough information for them to go on.
| if tt.toOrg { | ||
| defer reg.StubWithFixturePath(200, "./orgForkResult.json")() |
There was a problem hiding this comment.
Instead of switching stubs based on a boolean in the test case, consider letting the test case define its own stub.
| if tt.toOrg { | ||
| r := regexp.MustCompile(`Created fork.*batmanshome/REPO`) |
There was a problem hiding this comment.
Same here: consider letting the test case define its own expected output.
| "Added remote.*fork") | ||
| } | ||
|
|
||
| func TestRepoFork_in_parent_org_survey_no(t *testing.T) { |
There was a problem hiding this comment.
I appreciate you adding many extra tests, but all of these extra tests are completely unnecessary. The feature you've added is just unconditionally passing one extra parameter to the fork function. A single tests above is enough to exercise this functionality.
This commit adds functionality to fork a repository into an organization.
Closes #1247