Skip to content

Add missing fields to SecurityAdvisoryEvent and rename others#2889

Merged
gmlewis merged 2 commits intogoogle:masterfrom
nikpivkin:fix/security-adv
Aug 29, 2023
Merged

Add missing fields to SecurityAdvisoryEvent and rename others#2889
gmlewis merged 2 commits intogoogle:masterfrom
nikpivkin:fix/security-adv

Conversation

@nikpivkin
Copy link
Copy Markdown
Contributor

@nikpivkin nikpivkin commented Aug 20, 2023

Add missing fields to SecurityAdvisoryEvent

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 20, 2023 •
edited
Loading

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
//
// 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
Copy Markdown
Collaborator

@gmlewis gmlewis Aug 20, 2023

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

@gmlewis gmlewis gmlewis approved these changes

+1 more reviewer

@WillAbides WillAbides WillAbides approved these changes

Reviewers whose approvals may not affect merge requirements

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.

3 participants

@nikpivkin @gmlewis @WillAbides