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
Add DeleteDeployment for Repositories #1555
Add DeleteDeployment for Repositories #1555
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #1555 +/- ##
==========================================
- Coverage 67.92% 67.91% -0.01%
==========================================
Files 94 94
Lines 8670 8677 +7
==========================================
+ Hits 5889 5893 +4
- Misses 1880 1882 +2
- Partials 901 902 +1
Continue to review full report at Codecov.
|
4837902
to
0499296
Compare
0499296
to
78dfc53
Compare
This looks great, @nomonamo - thank you! Please make sure to sign the Google CLA (see instructions above), and then we can get it approved and merged. |
@googlebot I signed it! |
@googlebot I signed it! |
@willnorris - when you get a chance, could you please give @nomonamo some guidance on getting the CLA issues straightened out? Thank you! |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Took us some time to get the "Corporate signer" CLA right :) |
I'm glad you got it fixed. We use the codecov tool as a guideline and not a strict blocker so you do not need to be concerned about it. Ideally, we will increase the code coverage by writing unit tests that cover more code paths. |
t.Error("Repositories.DeleteDeployment should return an error") | ||
} | ||
if resp.StatusCode != http.StatusNotFound { | ||
t.Error("Repositories.DeleteDeployment should return a 404 status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the API docs, it looks like it should return 204 No Content
on a successful request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @nightlark.
@nomonamo - once you add the line above, then this can be changed to http.StatusNoContent
and the 404 can be changed to 204.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I was just adding a test for a non-existent deployment (id: 2), in an attempt to increase test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor tweak, @nomonamo and then I think we will be good to get a second LGTM and then merge.
|
||
mux.HandleFunc("/repos/o/r/deployments/1", func(w http.ResponseWriter, r *http.Request) { | ||
testMethod(t, r, "DELETE") | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
testMethod(t, r, "DELETE")
w.WriteHeader(http.StatusNoContent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add a test for that status
t.Error("Repositories.DeleteDeployment should return an error") | ||
} | ||
if resp.StatusCode != http.StatusNotFound { | ||
t.Error("Repositories.DeleteDeployment should return a 404 status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @nightlark.
@nomonamo - once you add the line above, then this can be changed to http.StatusNoContent
and the 404 can be changed to 204.
Thank you @nightlark and @gmlewis for reviewing 🙏 I've addressed your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @nomonamo and @nightlark!
LGTM.
Awaiting second LGTM before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👌
Thank you, @wesleimp! |
Addresses #1554