cloudevents
Replace time.Time.String() with time.Time.Format()
Resolves #987 Earlier, we used time.Time.String() to marshal time into a string. According to the go time.Time documentation however, time.Time.String() is only suitable for debugging purposes. This PR replaces time.Time.String with time.Time.Format() with RFC3339 to provide a stable serialized representation of time.
@MaryamTaj converted to draft, seems you're still working on this. Once ready for review, please just mark as "Ready for review" and ping us.
@duglin Hi Doug! I was running into some issues earlier, where all the tests pass locally but not on GitHub. I am going to try a new approach, and I'll let you know how it goes
@duglin I think I'm running into a wall. I tried to resolve as many errors as I could. Below, I have outlined the errors I am still running into, and what I have tried so far. This way, anyone else opening this PR does not have to start from scratch. I hope that will help, and I am genuinely sorry for not being able to resolve the issue! https://docs.google.com/document/d/1xfac9mAtsvB8RvqKN-Fz4F96W-VfdtivWGZitTi_QZg/edit?usp=sharing
@MaryamTaj I push a commit - see what you think. Also, I think you need to sign your commit(s). Might want to squash 'em. Did you add a testcase that checks the actual syntax of the "time" field?
@duglin Thank you so much for helping me out! I'm going through, and understanding how you resolved the issue. I will sign my commits to pass the DCO.
Glad it's sorted out! Before we merge, can you please squash/clean up commits into one?
@duglin do we consider this PR a breaking change for users? I understand our code was broken, so this would be a fix - but still could be an issue impacting downstream users?
Glad it's sorted out! Before we merge, can you please squash/clean up commits into one?
Yes it is a breaking change. Some options: 1 - merge it and see what happens :-) 2 - add a config option to allow people to turn it on if they wish, but no one will do so, or even know about it 3 - add a config option to get the current (wrong) behavior and only those who are broken will need to turn it on - if they don't want to be spec compliant going forward, but at least they have an immediate "fix" until they can "do the right thing"
I like 3.
Also, I think we need a test that checks the serialization of the timestamp IS actually the right format, not just that the sent and received values look the same. Unless I'm missing a test that already covers this.
I signed off and squashed all the commits into one. I also went through the codebase. Would v2/types/timestamp_test.go check the serialization of the timestamp is actually the right format?
I signed off and squashed all the commits into one. I also went through the codebase. Would v2/types/timestamp_test.go check the serialization of the timestamp is actually the right format?
I'm guessing no since you didn't need to change any test in there as part of this PR. At least some test should have failed as a result of your change... hence I think we need a new test that would pass w/o your change in v2/event/event_marshal.go but would fail with it.... then you can "fix" the test to fail w/o your change, and pass with it.
Yes, the existing tests might not detect it because if the producer is updated with the new semantics, the consumer will see automatically the new behavior. A test showing a producer with the old semantics and a consumer with the new behavior should fail with this change IIUC.
I also like option (3), but strictly speaking it requires a MAJOR version change in our semver scheme which doesn't feel justified. And my concern is that at the scale this SDK is used, consumers might not detect it right away?
I also like option (3), but strictly speaking it requires a MAJOR version change in our semver scheme which doesn't feel justified. And my concern is that at the scale this SDK is used, consumers might not detect it right away?
I agree that a major version bump doesn't feel right, especially if we want to consider this a "fix" rather than a new feature. One option is to do 2 and 3, meaning:
- add a flag to control which timestamp format to use, but keep the default as today's format (option 2)
- give lots of warnings in the docs, READMEs... about the issue and that the default will change
- create a new release
- then switch the default value of the flag in the next release (option 3)
At least then people had warnings and could migrate to the new format (turn on the flag) in prep for the final step at their own pace.
@duglin I don't see agreement on how we want to handle the breaking change, only suggestions. Seems like we want to merge and point out in the release notes?
Oops - I forgot about that. I'm still leaning toward 3 (pull the band-aide off but give people a way to get the old behavior, at least for a while).
I'll work on adding a flag to give the old behavior, but won't add it to the PR until you agree with that direction.
While trying to write a testcase for the new flag I'm unable to reproduce any error. I always see the right time format. I'm starting to think this isn't an issue. See: https://github.com/cloudevents/sdk-go/issues/987#issuecomment-2745937686 and the next comment.