Support breaking changes to the Projects API preview

[gmlewis]

GitHub Announcement:
https://developer.github.com/changes/2017-09-01-breaking-changes-to-projects-api-preview-and-webhooks-date-format/

To resolve this issue, it appears that unit tests will need to be updated for the time parsing functions that process webhook payloads. It is possible that the new format is already supported, but it is more likely that the parsing format needs to be changed slightly.

I personally think that it might be nice to support both formats (old and new) in the code base with a "TODO" to remove support for the old format once it is no longer being sent by GitHub, but this is completely up for discussion and I don't have my heart set on this. (One nice serendipity of this would be that no breaking changes would be made to the API.)

Thank you in advance for contributing to this open source project!
Your assistance is greatly appreciated.

[dmitshur]

"TODO" to remove support for the old format once it is no longer being sent by GitHub

But GitHub only sends at most one format at a time, never both. We can opt-in to the new format earlier by using a preview header. The date the list is when the old format will stop being available and will become the default even without the preview header.

So if you want to to support old format while having the new format, we'd have to compute the old one locally.

I'd prefer a more instantaneous change. People will need to change their code eventually regardless. If they want to wait, they can use an older version of go-github for a while longer. There's very little benefit in allowing people to use old format with latest version of go-github, it just delays the inevitable.

One nice serendipity of this would be that no breaking changes would be made to the API.

Unless I'm missing something, a breaking API change is unavoidable. The only thing we can control is whether it happens on October 1st, 2017 or sooner.

[gmlewis]

If I understand correctly, GitHub is only changing the format of their webhook timestamps... and our API returns time.Time anyway, so I'm thinking/hoping that no API from a Go standpoint will be broken... but I could be wrong.

[dmitshur]

Ah, indeed. We're likely using our Timestamp type that handles both.

That's great, lucky us! :)

[elliott-beach]

Great! Please assign to me.

[elliott-beach]

Interestingly, the created_at and updated_at fields for Projects are populated in exactly the same way as for timestamps in the rest of the API. Perhaps for this reason, there is no test covering the irregular timestamp format.

I'll add a unit test covering the timestamp format after the change, although, logically, it shouldn't make any difference, because all the timestamp logic is handled by the json package right here:

go-github/github/github.go

Line 458 in 12126fb

err = json.NewDecoder(resp.Body).Decode(v)

[dmitshur]

@e-beach You're probably already aware, but we already have some unit tests for Timestamp type in timestamp_test.go file, so that's where new unit tests (if any are needed) would go. Thanks. :)

[elliott-beach]

I was not aware, thanks!

[elliott-beach]

The revised timestamp format is already covered by the tests in timestamp.go:

go-github/github/timestamp_test.go

Lines 15 to 19 in 12126fb

	const (
	emptyTimeStr = `"0001-01-01T00:00:00Z"`
	referenceTimeStr = `"2006-01-02T15:04:05Z"`
	referenceUnixTimeStr = `1136214245`
	)

Based on this, I don't believe any changes to code/tests need to be made to cover the change.

We could add a test to cover the format currently returned by the Projects API, which contains milliseconds, e.g.,"2017-08-24T15:56:15.000Z".

[dmitshur]

The revised timestamp format is already covered by the tests in timestamp.go ... Based on this, I don't believe any changes to code/tests need to be made to cover the change.

Sounds good. Thanks!

We could add a test to cover the format currently returned by the Projects API, which contains milliseconds, e.g.,"2017-08-24T15:56:15.000Z".

That sounds like an improvement. Want to send a PR for that? Mention in the test case where the value comes from (in this case, some Projects API endpoint).

[gmlewis]

I know this is a closed issue, but just for completeness, I wanted to include the latest GitHub Developer's API announcement:
https://developer.github.com/changes/2017-10-02-breaking-changes-to-projects-api-preview-and-webhooks-date-format/
Once again, this should not affect anything.
Carry on. 😄