Installation: Use *Timestamp instead of *time.Time for the benefit of InstallationEvent by bmoylan · Pull Request #965 · google/go-github · GitHub
Skip to content

Installation: Use *Timestamp instead of *time.Time for the benefit of InstallationEvent#965

Merged
gmlewis merged 1 commit into
google:masterfrom
bmoylan:bm/installation-timestamp
Aug 6, 2018
Merged

Installation: Use *Timestamp instead of *time.Time for the benefit of InstallationEvent#965
gmlewis merged 1 commit into
google:masterfrom
bmoylan:bm/installation-timestamp

Conversation

@bmoylan

@bmoylan bmoylan commented Aug 5, 2018

Copy link
Copy Markdown
Contributor

According to https://developer.github.com/v3/activity/events/types/#installationevent the created_at and updated_at timestamps in the event payloads are unix-formatted, while the CRUD APIs use RFC3339. Using *Timestamp handles the unfortunate fact that the API is inconsistent. Without this change, my application emits the following error

failed to unmarshal PullRequestEvent: parsing time "1533435666" as ""2006-01-02T15:04:05Z07:00"": cannot parse "1533435666" as """

I understand this might be a breaking change to the public API, so I'm open to feedback if there are alternative ways to proceed.


This change is Reviewable

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Aug 5, 2018
@bmoylan

bmoylan commented Aug 5, 2018

Copy link
Copy Markdown
Contributor Author

@gmlewis gmlewis 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, @bmoylan!

You definitely found a bug, so we have no problems breaking the API when the current implementation doesn't work. We will need to label this repo with a new version for the breaking change.

LGTM.
Awaiting second LGTM before merging.

@gmlewis gmlewis requested a review from juliaferraioli August 5, 2018 12:29

@juliaferraioli juliaferraioli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice catch, @bmoylan!

LGTM.

@gmlewis

gmlewis commented Aug 6, 2018

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis merged commit 2f5e75e into google:master Aug 6, 2018
@bmoylan bmoylan deleted the bm/installation-timestamp branch August 6, 2018 17:51
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Aug 10, 2018
PatWie added a commit to PatWie/pylint that referenced this pull request Aug 14, 2018
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants