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

Change type of HookDelivery.InstallationID to Int64. #2235

Merged

Conversation

pierce-m
Copy link
Contributor

Resolves #2234

@codecov
Copy link

codecov bot commented Dec 17, 2021

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 Files Coverage Δ
github/repos_hooks_deliveries.go 90.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.

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.

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

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.

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!

@gmlewis gmlewis 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
Copy link
Contributor 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! 👍

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

Awaiting second LGTM from any other repo contributor before merging.

@pierce-m
Copy link
Contributor Author

pierce-m commented Jan 3, 2022

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

Copy link

@Parker77 Parker77 left a comment

Choose a reason for hiding this comment

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

LGTM.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 3, 2022

Thank you, @Parker77 !
Merging.

@gmlewis gmlewis merged commit f334d81 into google:master Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Incorrect type in HookDelivery
3 participants