Handle protection status errors for unprotected branches #2092
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #2092 +/- ##
==========================================
+ Coverage 97.75% 97.78% +0.03%
==========================================
Files 107 109 +2
Lines 9601 9733 +132
==========================================
+ Hits 9385 9517 +132
Misses 150 150
Partials 66 66
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you, @hemachandarv !
Just a few minor tweaks, please.
github/repos_test.go
Outdated
|
|
||
| w.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprintf(w, `{ | ||
| "message": "Branch not protected", |
There was a problem hiding this comment.
| "message": "Branch not protected", | |
| "message": githubBranchNotProtected, |
There was a problem hiding this comment.
@gmlewis I'm not sure if we'll be able to use a variable inside a raw string. Am I missing something..?
There was a problem hiding this comment.
Whups, you are correct. Please use %v and the variable name in that case.
There was a problem hiding this comment.
Actually, %q makes more sense here.
There was a problem hiding this comment.
Thanks @gmlewis. I've updated the PR with the suggested changes.
github/repos_test.go
Outdated
|
|
||
| w.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprintf(w, `{ | ||
| "message": "Branch not protected", |
There was a problem hiding this comment.
| "message": "Branch not protected", | |
| "message": githubBranchNotProtected, |
github/repos_test.go
Outdated
|
|
||
| w.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprintf(w, `{ | ||
| "message": "Branch not protected", |
There was a problem hiding this comment.
| "message": "Branch not protected", | |
| "message": githubBranchNotProtected, |
github/repos.go
Outdated
| if errorResponse, ok := err.(*ErrorResponse); ok { | ||
| return errorResponse.Message == githubBranchNotProtected | ||
| } | ||
| return false |
There was a problem hiding this comment.
| if errorResponse, ok := err.(*ErrorResponse); ok { | |
| return errorResponse.Message == githubBranchNotProtected | |
| } | |
| return false | |
| errorResponse, ok := err.(*ErrorResponse) | |
| return ok && errorResponse.Message == githubBranchNotProtected |
There was a problem hiding this comment.
@gmlewis Thank you for suggesting this change. I certainly missed this simplification.
There was a problem hiding this comment.
Thank you, @hemachandarv !
LGTM.
Awaiting second LGTM before merging.
|
Thank you, @cpanato ! |
Resolves #625