-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Implement global security advisories API #2993
Conversation
aebb733
to
0a04270
Compare
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.
Thanks, @cpanato. I'm going to stop my code review early here and ask you to remove as many new structs that you've added as possible, and to instead reuse SecurityAdvisory
and other related structs as much as possible without all the duplication.
Also, every new exported struct that you add needs a full godoc string.
Signed-off-by: cpanato <ctadeu@gmail.com>
Codecov ReportAttention:
Additional details and impacted files
☔ View full report in Codecov by Sentry. |
Signed-off-by: cpanato <ctadeu@gmail.com>
@gmlewis thanks so much for your review and sorry about the force-push |
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.
OK, two small nits, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
Signed-off-by: cpanato <ctadeu@gmail.com>
@gmlewis Thanks again for your review, PTAL |
Signed-off-by: cpanato <ctadeu@gmail.com>
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, @cpanato !
LGTM after one tiny nit, after which we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
Signed-off-by: cpanato <ctadeu@gmail.com>
Signed-off-by: cpanato <ctadeu@gmail.com>
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, @cpanato !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
@gmlewis checking if you know how long this will take? :) |
@cpanato - we rely on volunteers to help maintain this repo. I don't know when another contributor will be available to review this PR before merging. And yes, we should be able to make a new release soon as well. |
thank you @gmlewis, no worries just checking :) |
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, @valbeat ! |
thanks for all the support |
Hi @cpanato - this is now released here: https://github.com/google/go-github/releases/tag/v57.0.0 |
No sorry is needed. Thank you for all the work you do here 🎉 and thanks for the release |
Fixes: #2851
cc @gmlewis