Skip to content

Add support for enable/disable private vulnerability reporting on repositories - #2887

Merged
gmlewis merged 2 commits into
google:masterfrom
vandanrohatgi:issue2883
Aug 20, 2023
Merged

Add support for enable/disable private vulnerability reporting on repositories#2887
gmlewis merged 2 commits into
google:masterfrom
vandanrohatgi:issue2883

Conversation

@vandanrohatgi

Copy link
Copy Markdown
Contributor

Closes#2883

  • go generate
  • go vet
  • go test

Please let me know if the method names are not descriptive enough or just need to be changed.

Also, something that's been bugging me. In my previous PR, I did some actual testing aside from the unit tests. While not present in CONTRIBUTING.md , is it advisable/necessary/optional to perform actual testing (importing the library, calling the new method and examine response).

@codecov

codecovBot commented Aug 19, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2887 (4338ba0) into master (a0e8f35) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2887 +/- ##
=======================================
Coverage 98.06% 98.06% =======================================
Files 140 140 Lines 12269 12293 +24 =======================================
+ Hits 12031 12055 +24 
Misses 162 162 Partials 76 76 
Files ChangedCoverage Δ
github/repos.go98.89% <100.00%> (+0.03%)⬆️

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @vandanrohatgi !
A few minor tweaks, please, then we will be ready for a second LGTM+Approval from any other contributor to this repo before merging.

Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Aug 19, 2023
@gmlewis

gmlewis commented Aug 19, 2023

Copy link
Copy Markdown
Collaborator

Also, something that's been bugging me. In my previous PR, I did some actual testing aside from the unit tests. While not present in CONTRIBUTING.md , is it advisable/necessary/optional to perform actual testing (importing the library, calling the new method and examine response).

In this repo, we do not typically add to the integration tests anymore.
They were extremely helpful in the early design of the repo, but in my opinion, the surface area of this API is so huge that it would be an onerous task to attempt to keep up-to-date in addition to the unit tests.

Having said that, we place a high value on unit tests in this repo and want every endpoint to be exercised "round-trip" with an in-memory http server to verify that every one looks like it should work in real practice.

So you are proceeding along just as planned, and we really appreciate your contributions, @vandanrohatgi !

Keep up the great work! 😁 ❤️

Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@vandanrohatgi

Copy link
Copy Markdown
ContributorAuthor

Thank you for making this project such a pleasure to work with!

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @vandanrohatgi !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@mnafees

Copy link
Copy Markdown
Contributor

LGTM 👍🏽

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 20, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @mnafees !
Merging.

@gmlewis
gmlewis merged commit 7ceef94 into google:masterAug 20, 2023
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
@vandanrohatgi
vandanrohatgi deleted the issue2883 branch December 5, 2023 06:31
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support enable or disable private vulnerability reporting on repositories

3 participants

@vandanrohatgi@gmlewis@mnafees