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.

Sign up for GitHub

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

Jump to bottom

Add missing fields to SecurityAdvisoryEvent and rename others #2889

Merged
merged 2 commits into from Aug 29, 2023
Merged

Add missing fields to SecurityAdvisoryEvent and rename others #2889

merged 2 commits into from Aug 29, 2023

Conversation

nikpivkin
Copy link
Contributor

Add missing fields to SecurityAdvisoryEvent

@codecov
Copy link

codecov bot commented Aug 20, 2023 •
edited

Codecov Report

Merging #2889 (2d733ae) into master (a0e8f35) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is n/a.

❗ Current head 2d733ae differs from pull request most recent head 09b8437. Consider uploading reports for the commit 09b8437 to get more accurate results

@@           Coverage Diff           @@
##           master    #2889   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         140      140           
  Lines       12269    12269           
=======================================
  Hits        12031    12031           
  Misses        162      162           
  Partials       76       76
Files Changed Coverage Δ
github/event_types.go 100.00% <ø> (ø)

gmlewis
gmlewis reviewed Aug 20, 2023
View reviewed changes
github/event_types.go Outdated
@@ -1591,6 +1591,8 @@ type WorkflowRunEvent struct {
//
// GitHub API docs: https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#security_advisory
type SecurityAdvisory struct {
CVSs *AdvisoryCVSs `json:"cvss,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't do a full review right now, but I noticed that cvss is singular (not plural) and is an acronym for "CVS Score" so its capitalization should be "CVSS" .

The same is true for AdvisoryCVSS.

@nikpivkin nikpivkin requested a review from gmlewis August 21, 2023 14:51
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 21, 2023

Ah! I see that you are fixing some legacy naming mistakes. Thank you, @nikpivkin !
Marking this PR as "breaking API changes" out of necessity.

@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Aug 21, 2023
gmlewis
gmlewis approved these changes Aug 21, 2023
View reviewed changes
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, @nikpivkin !
LGTM.

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

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

gmlewis commented Aug 29, 2023

Thank you, @WillAbides !
Merging.

@gmlewis gmlewis changed the title Add missing fields to SecurityAdvisoryEvent Add missing fields to SecurityAdvisoryEvent and rename others Aug 29, 2023
@gmlewis gmlewis merged commit b9774ad into google:master Aug 29, 2023
7 checks passed
@gmlewis gmlewis mentioned this pull request Sep 5, 2023
Merged
This was referenced Sep 14, 2023
Merged
Merged
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
@nikpivkin @gmlewis
Add missing fields to SecurityAdvisoryEvent and rename others (google…
6105943 
…#2889)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAbides WillAbides WillAbides approved these changes

@gmlewis gmlewis gmlewis approved these changes

Assignees
No one assigned
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nikpivkin @gmlewis @WillAbides