Move to go 1.18. #958
Move to go 1.18. #958
Conversation
.github/workflows/conformance.yaml
Outdated
| @@ -12,7 +12,7 @@ jobs: | |||
| name: CloudEvents | |||
| strategy: | |||
| matrix: | |||
| go-version: [1.17.x] | |||
| go-version: [1.18.x] | |||
There was a problem hiding this comment.
nit: you can use the latest available go version in the workflows
There was a problem hiding this comment.
ok I changed them all to use "stable".
There was a problem hiding this comment.
I think one downside to this is that the first person to do a PR after a new release of golang might be forced to do things like run "gofmt" on everything if the format rules change. Just ran into that. But let's see how it goes...
There was a problem hiding this comment.
Yeah, not a big fan of these abstract version identifiers, especially when it comes to troubleshooting and you don't see immediately which version we are using in CI. Feel free to revert to 1.21 for now.
duglin
force-pushed
the
go1.18
branch
2 times, most recently
from
99f1dab
to
a702319
Compare
duglin
force-pushed
the
go1.18
branch
2 times, most recently
from
2fd7a51
to
9e22de0
Compare
|
with the race condition behind us, this one is now ready for final review and merge. |
v2/types/value.go
Outdated
| @@ -86,7 +86,8 @@ func Format(v interface{}) (string, error) { | |||
| } | |||
|
|
|||
| // Validate v is a valid CloudEvents attribute value, convert it to one of: | |||
| // bool, int32, string, []byte, types.URI, types.URIRef, types.Timestamp | |||
| // | |||
There was a problem hiding this comment.
question: is this newline intentional?
There was a problem hiding this comment.
I have no idea how this got here aside from gofmt doing it - I would not have added this. I'll remove it and see what happens
There was a problem hiding this comment.
I think there were tabs in the comments and gofmt converted them to blank lines
v2/protocol/http/context.go
Outdated
| @@ -24,7 +24,8 @@ type RequestData struct { | |||
| } | |||
|
|
|||
| // WithRequestDataAtContext uses the http.Request to add RequestData | |||
| // information to the Context. | |||
| // | |||
| // information to the Context. | |||
| @@ -29,6 +30,9 @@ func SendReceive(t *testing.T, protocolFactory func() interface{}, in event.Even | |||
| wg := sync.WaitGroup{} | |||
| wg.Add(2) | |||
|
|
|||
| // Give time for Kafka client protocol to get setup | |||
| time.Sleep(5 * time.Second) | |||
There was a problem hiding this comment.
question: why did we add those sleeps in this PR?
There was a problem hiding this comment.
Because I had this PR open :-)
I can create a separate PR but in the end does it really matter as long as it all gets merged?
| @@ -104,7 +104,7 @@ func testSenderReceiver(t testing.TB) (func(), bindings.Sender, bindings.Receive | |||
|
|
|||
| // Not perfect but we need to give OpenInbound() as chance to start | |||
| // as it's a race condition. I couldn't find something on 'p' to wait for | |||
| time.Sleep(5 * time.Second) | |||
| time.Sleep(15 * time.Second) | |||
There was a problem hiding this comment.
question: why did we change this in this PR?
There was a problem hiding this comment.
once i added the other sleeps I saw this race condition appear again. So I increased this one to be more than the other 2 sleep values combined. Not 100% sure if I'm just getting lucky or not but my local testing seems happier now
duglin
force-pushed
the
go1.18
branch
2 times, most recently
from
9e42e52
to
30fd973
Compare
Had to run gofmt and fix some weird typos due to tabs in the comments Signed-off-by: Doug Davis <[email protected]>
Signed-off-by: Doug Davis <[email protected]>
Upgrade to go v1.18
Had to run gofmt on a few files too.
Added more
time.Sleepcalls to try to fix the race condition, again.