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.

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

Add "organization" field to more events #2949

Merged
merged 6 commits into from
Oct 4, 2023
Merged

Add "organization" field to more events #2949

merged 6 commits into from
Oct 4, 2023

Conversation

billnapier
Copy link
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

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
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
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
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
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
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
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
7 checks passed
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