Skip to content

Change type of HookDelivery.InstallationID to Int64. - #2235

Merged
gmlewis merged 2 commits into
google:masterfrom
watchtowerai:hookdelivery-installationid-to-number
Jan 3, 2022
Merged

Change type of HookDelivery.InstallationID to Int64.#2235
gmlewis merged 2 commits into
google:masterfrom
watchtowerai:hookdelivery-installationid-to-number

Conversation

@pierce-m

@pierce-mpierce-m commented Dec 17, 2021

Copy link
Copy Markdown

Resolves#2234

@codecov

codecovBot commented Dec 17, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2235 (8ba2c80) into master (1d9884e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #2235 +/- ##
=======================================
Coverage 97.79% 97.79% =======================================
Files 113 113 Lines 10156 10156 =======================================
Hits 9932 9932 Misses 156 156 Partials 68 68
Impacted FilesCoverage Δ
github/repos_hooks_deliveries.go90.24% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d9884e...8ba2c80. Read the comment docs.

gmlewis
gmlewis reviewed Dec 18, 2021
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.

I'm wondering why these fields are pointers but don't have omitempty?

gmlewis
gmlewis requested changes Dec 18, 2021
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.

I've looked through the code and can't find an explanation for why the fields in the HookDelivery do not have an omitempty even though they are all references.

So while we are breaking the API, please go ahead and add omitempty to all the fields in lines 20-30. Thank you!

@gmlewisgmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Dec 18, 2021
@pierce-m

pierce-m commented Dec 21, 2021

Copy link
Copy Markdown
Author

@gmlewis yeah it looks like it was added on the Request and Response fields 🤔, maybe because this struct isn't used as input (as far as I can tell)? Anyways, added! 👍

gmlewis
gmlewis approved these changes Dec 21, 2021
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, @pierce-m !
LGTM.

Awaiting second LGTM from any other repo contributor before merging.

@pierce-m

pierce-m commented Jan 3, 2022

Copy link
Copy Markdown
Author

Is there anyone else who can provide review? Doesn't seem like @wesleimp is very active any more.

Parker77
Parker77 approved these changes Jan 3, 2022
View reviewed changes

@Parker77Parker77 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM.

@gmlewis

gmlewis commented Jan 3, 2022

Copy link
Copy Markdown
Collaborator

Thank you, @Parker77 !
Merging.

@gmlewis
gmlewis merged commit f334d81 into google:masterJan 3, 2022
@gmlewisgmlewis mentioned this pull request Jan 13, 2022
Merged
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
@jlaportebot
Change type of HookDelivery.InstallationID to int64 (google#2235)
5d8c586
Fixes: google#2234.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewisgmlewisgmlewis approved these changes
@wesleimpwesleimpAwaiting requested review from wesleimp
+1 more reviewer
@Parker77Parker77Parker77 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.

Incorrect type in HookDelivery

3 participants

@pierce-m@gmlewis@Parker77