Skip to content

Add missing fields to SecurityAdvisoryEvent and rename others - #2889

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

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

Conversation

@nikpivkin

@nikpivkinnikpivkin commented Aug 20, 2023

Copy link
Copy Markdown
Contributor

Add missing fields to SecurityAdvisoryEvent

@codecov

codecovBot commented Aug 20, 2023•
edited
Loading

Copy link
Copy Markdown

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 ChangedCoverage Δ
github/event_types.go100.00% <ø> (ø)

gmlewis
gmlewis reviewed Aug 20, 2023
View reviewed changes
Comment threadgithub/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"`

@gmlewisgmlewisAug 20, 2023

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.

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 gmlewisAugust 21, 2023 14:51
@gmlewis

gmlewis commented Aug 21, 2023

Copy link
Copy Markdown
Collaborator

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

@gmlewisgmlewis 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

@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, @nikpivkin !
LGTM.

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

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

gmlewis commented Aug 29, 2023

Copy link
Copy Markdown
Collaborator

Thank you, @WillAbides !
Merging.

@gmlewisgmlewis changed the title Add missing fields to SecurityAdvisoryEventAdd missing fields to SecurityAdvisoryEvent and rename othersAug 29, 2023
@gmlewis
gmlewis merged commit b9774ad into google:masterAug 29, 2023
@gmlewisgmlewis 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)
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
@jlaportebot
Add missing fields to SecurityAdvisoryEvent and rename others (google…
8f8512f
…#2889)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewisgmlewisgmlewis approved these changes
+1 more reviewer
@WillAbidesWillAbidesWillAbides approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Breaking API ChangePR 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