Add DeleteDeployment for Repositories by pablito-perez · Pull Request #1555 · google/go-github

pablito-perez · 2020-06-09T14:23:13Z

Addresses #1554

googlebot · 2020-06-09T14:23:16Z

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 @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

codecov · 2020-06-09T14:26:27Z

Codecov Report

Merging #1555 into master will decrease coverage by 0.00%.
The diff coverage is 57.14%.

@@            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

Impacted Files	Coverage Δ
github/repos_deployments.go	`56.31% <57.14%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbeb049...e69b5e3. Read the comment docs.

gmlewis · 2020-06-10T03:14:24Z

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.

pablito-perez · 2020-06-10T07:58:18Z

@googlebot I signed it!

pablito-perez · 2020-06-10T08:06:13Z

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 @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).

The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.

The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

@googlebot I signed it!

gmlewis · 2020-06-10T12:45:59Z

@willnorris - when you get a chance, could you please give @nomonamo some guidance on getting the CLA issues straightened out? Thank you!

pablito-perez · 2020-06-11T07:30:08Z

@googlebot I signed it!

googlebot · 2020-06-11T07:30:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

pablito-perez · 2020-06-11T07:31:39Z

@willnorris - when you get a chance, could you please give @nomonamo some guidance on getting the CLA issues straightened out? Thank you!

Took us some time to get the "Corporate signer" CLA right :)
Thanks for your comment @gmlewis ! I'm not sure what to do about the checks from codecov though?

gmlewis · 2020-06-11T11:58:21Z

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.

nightlark · 2020-06-13T16:47:16Z

+		t.Error("Repositories.DeleteDeployment should return an error")
+	}
+	if resp.StatusCode != http.StatusNotFound {
+		t.Error("Repositories.DeleteDeployment should return a 404 status")


From the API docs, it looks like it should return 204 No Content on a successful request?

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.

In this case I was just adding a test for a non-existent deployment (id: 2), in an attempt to increase test coverage.

gmlewis

Just one minor tweak, @nomonamo and then I think we will be good to get a second LGTM and then merge.

gmlewis · 2020-06-14T03:11:24Z

+
+	mux.HandleFunc("/repos/o/r/deployments/1", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "DELETE")
+	})


This should be:

testMethod(t, r, "DELETE") w.WriteHeader(http.StatusNoContent)

Sure, I'll add a test for that status

gmlewis · 2020-06-14T03:12:17Z

+		t.Error("Repositories.DeleteDeployment should return an error")
+	}
+	if resp.StatusCode != http.StatusNotFound {
+		t.Error("Repositories.DeleteDeployment should return a 404 status")


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.

pablito-perez · 2020-06-15T07:17:55Z

Thank you @nightlark and @gmlewis for reviewing 🙏 I've addressed your comments

gmlewis

Thank you, @nomonamo and @nightlark!
LGTM.

Awaiting second LGTM before merging.

wesleimp

LGTM 👌

gmlewis · 2020-06-17T01:11:04Z

Thank you, @wesleimp!
Merging.

Fixes: google#1554.

googlebot added the cla: no label Jun 9, 2020

pablito-perez force-pushed the repositories-delete-deployment branch 2 times, most recently from 4837902 to 0499296 Compare June 9, 2020 14:27

Add DeleteDeployment for Repositories

78dfc53

pablito-perez force-pushed the repositories-delete-deployment branch from 0499296 to 78dfc53 Compare June 9, 2020 14:29

test for DeleteDeployment failure

0b1d8cc

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Jun 11, 2020

nightlark reviewed Jun 13, 2020

View reviewed changes

gmlewis requested changes Jun 14, 2020

View reviewed changes

gmlewis requested review from gauntface and wesleimp June 14, 2020 03:13

successful delete request returns 204

e69b5e3

pablito-perez requested a review from gmlewis June 15, 2020 07:15

gmlewis approved these changes Jun 15, 2020

View reviewed changes

wesleimp approved these changes Jun 16, 2020

View reviewed changes

gmlewis merged commit 3312662 into google:master Jun 17, 2020

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Add DeleteDeployment for Repositories (google#1555)

a6965d0

Fixes: google#1554.

Conversation

pablito-perez commented Jun 9, 2020

Uh oh!

googlebot commented Jun 9, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

codecov Bot commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis commented Jun 10, 2020

Uh oh!

pablito-perez commented Jun 10, 2020

Uh oh!

pablito-perez commented Jun 10, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

gmlewis commented Jun 10, 2020

Uh oh!

pablito-perez commented Jun 11, 2020

Uh oh!

googlebot commented Jun 11, 2020

Uh oh!

pablito-perez commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Jun 11, 2020

Uh oh!

nightlark Jun 13, 2020

Choose a reason for hiding this comment

Uh oh!

gmlewis Jun 14, 2020

Choose a reason for hiding this comment

Uh oh!

pablito-perez Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis Jun 14, 2020

Choose a reason for hiding this comment

Uh oh!

pablito-perez Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

gmlewis Jun 14, 2020

Choose a reason for hiding this comment

Uh oh!

pablito-perez commented Jun 15, 2020

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

wesleimp left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Jun 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Jun 9, 2020 •

edited

Loading

pablito-perez commented Jun 11, 2020 •

edited

Loading