Skip to content

Add "organization" field to more events#2949

Merged
gmlewis merged 6 commits intogoogle:masterfrom
billnapier:add_orgs
Oct 4, 2023
Merged

Add "organization" field to more events#2949
gmlewis merged 6 commits intogoogle:masterfrom
billnapier:add_orgs

Conversation

@billnapier
Copy link
Copy Markdown
Contributor

@billnapier billnapier commented Oct 3, 2023

This should cover all events that GitHub says have an organization field.

Fixes #2950

This should cover all events that GitHub says have an organization field.
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 3, 2023

Codecov Report

Merging #2949 (3606947) into master (8cd452b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2949   +/-   ##
=======================================
  Coverage   98.19%   98.19%           
=======================================
  Files         145      145           
  Lines       12720    12720           
=======================================
  Hits        12490    12490           
  Misses        156      156           
  Partials       74       74           
Files Coverage Δ
github/event_types.go 100.00% <ø> (ø)


// The following field is only present when the webhook is triggered on
// a repository belonging to an organization.
Organization *Organization `json:"organization,omitempty"`
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.

Let's please name them all Org.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I left the existing ones, as I wasn't sure if you wanted a breaking change here or not. Happy to go change them all if you don't mind the breakage.

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.

Oh, sorry! That will teach me to do code reviews on my Android phone. 😂

Hmmm... consistency is nice as long as we document our changes.

Sure, let's be consistent and I'll mark this as a breaking API change. 😁

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 4, 2023

Choose a reason for hiding this comment

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

Hmmm... I'm no longer on my phone, and I'm not understanding this statement:

I left the existing ones, as I wasn't sure if you wanted a breaking change here or not. Happy to go change them all if you don't mind the breakage.

I don't see any existing organization fields in any of the structs you modified, so I don't think this is a breaking API change afterall, correct?

This field is inconsistently named throughout this file, but
updating the old instances would be a breaking change, so they got left.
@gmlewis gmlewis added Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). NeedsReview PR is awaiting a review before merging. and removed Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Oct 3, 2023
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, @billnapier !
LGTM.

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

@gmlewis gmlewis mentioned this pull request Oct 4, 2023
3 tasks
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Oct 4, 2023
@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Oct 4, 2023

Thank you, @WillAbides !
Merging.

@gmlewis gmlewis changed the title Add "organization" field to more events. Add "organization" field to more events Oct 4, 2023
@gmlewis gmlewis merged commit 84651d1 into google:master Oct 4, 2023
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.

Not all Webhook events that provide organizations, are available in the library.

3 participants