Skip to content
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 Repository Security Advisories APIs #2902

Merged
merged 14 commits into from
Oct 5, 2023

Conversation

anishrajan25
Copy link
Contributor

@anishrajan25 anishrajan25 commented Aug 26, 2023

@google-cla
Copy link

google-cla bot commented Aug 26, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@anishrajan25 anishrajan25 marked this pull request as ready for review August 26, 2023 15:48
@anishrajan25 anishrajan25 force-pushed the addRepositorySecurityAdvisoryAPIs branch from 7285192 to bc0f60e Compare August 26, 2023 15:48
@gmlewis gmlewis changed the title Added Repository Security Advisories APIs Add Repository Security Advisories APIs Aug 26, 2023
@anishrajan25
Copy link
Contributor Author

@gmlewis The PR is ready for review. Could you please have a look as per your availability?

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

I don't have time for a full review right now, but at a quick glance, it looks like you are defining a whole bunch of new structs... possibly more than necessary. We try to reuse structs when the meaning is similar if possible.

It also looks like you are redefining ListOptions and/or ListCursorOptions when they could be embedded into other options... please look around this repo for examples on how to do this.

Third, all new structs must have complete sentences ending in a period for the auto-generated godocs to be formatted correctly. Please fix this.

Additionally, it looks like there are linting and/or test errors that need fixing. Please address.

@anishrajan25
Copy link
Contributor Author

anishrajan25 commented Sep 10, 2023

@gmlewis I have analysed the existing structs and prepared a document capturing the possibilities of embedding some structs to prevent redefining some fields.

Could you please have a look at this document and add comments wherever necessary to provide your inputs?

I will fix the comments to end with period, linting, and test errors in the next commit along with updating the struct definitions.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 10, 2023

I'll be back on 9/18 when I can take a look at this. Sorry for the delay and inconvenience.

@anishrajan25
Copy link
Contributor Author

No worries. You can have a look whenever you are available ✌🏼

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

@anishrajan25 - I'm back, but none of the comments I left above:
#2902 (review)
have been addressed. Please fix these issues before we can continue.

@anishrajan25
Copy link
Contributor Author

anishrajan25 commented Sep 30, 2023

Hi @gmlewis , apologies for the delay in response.

I have created this document, capturing the possibilities of embedding some structs to prevent redefining some fields.
Could you please have a look and add comments wherever necessary to provide your input?

@anishrajan25
Copy link
Contributor Author

I have updated the existing structs and reused them instead of creating new ones wherever possible. I have fixed the comments as well. @gmlewis Could you please have a look?

github/security_advisories.go Outdated Show resolved Hide resolved
github/security_advisories.go Outdated Show resolved Hide resolved
github/security_advisories.go Outdated Show resolved Hide resolved
github/security_advisories.go Outdated Show resolved Hide resolved
github/security_advisories.go Outdated Show resolved Hide resolved
github/security_advisories_test.go Outdated Show resolved Hide resolved
github/security_advisories_test.go Outdated Show resolved Hide resolved
github/security_advisories_test.go Outdated Show resolved Hide resolved
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 2, 2023

Please check that test pass locally with go test ./... before pushing.

@anishrajan25
Copy link
Contributor Author

There are some failing test cases. I will update once the test cases are fixed.

@anishrajan25
Copy link
Contributor Author

@gmlewis I have fixed the test cases as well. All the tests are passing on my local

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 3, 2023

Please run go generate ./... as described in CONTRIBUTING.md and push the results to this PR.

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #2902 (8d1daf2) into master (5224e34) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2902   +/-   ##
=======================================
  Coverage   98.17%   98.17%           
=======================================
  Files         145      145           
  Lines       12735    12767   +32     
=======================================
+ Hits        12502    12534   +32     
  Misses        158      158           
  Partials       75       75           
Files Coverage Δ
github/event_types.go 100.00% <ø> (ø)
github/security_advisories.go 100.00% <100.00%> (ø)

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Oct 4, 2023
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @anishrajan25 !
LGTM.

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

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Oct 5, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 5, 2023

Thank you, @valbeat !
Merging.

@gmlewis gmlewis merged commit 74db58f into google:master Oct 5, 2023
7 checks passed
Sign up for free to 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.

3 participants