-
Notifications
You must be signed in to change notification settings - Fork 616
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
Move periodic timeout implementation to wsclient library. #3883
Move periodic timeout implementation to wsclient library. #3883
Conversation
Looks mostly good, seems that majority of comments in the previous PR have been addressed! Just left a few comments based on the most recent changes made. |
@@ -42,7 +44,10 @@ import ( | |||
"github.com/stretchr/testify/require" | |||
) | |||
|
|||
const dockerEndpoint = "/var/run/docker.sock" | |||
const ( | |||
dockerEndpoint = "/var/run/docker.sock" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Probably change variable name as this is going to be shared by other container runtime socket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something which was preexisting actually. But sure, let me take a look up if it impacts and if it can be something random. Since, this is not blocking, I would pick it up in a later CR, if thats okay.
@@ -441,9 +440,6 @@ func TestClientReconnectsAfterInactiveTimeout(t *testing.T) { | |||
// it would continue to reconnect and test will be in forever loop. | |||
assert.False(t, websocket.IsCloseError(err, websocket.CloseAbnormalClosure), | |||
"Read from closed connection should produce an io.EOF error") | |||
|
|||
assert.Equal(t, err.Error(), context.DeadlineExceeded.Error(), "Context deadline exceeded error expected.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (non-blocking): Just curious, is there a specific reason why this assert statement was removed from this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the test was flaky. The return code differs based on when the ctx is cancelled. If the context is cancelled when we are in client.consumeMessages, it results in non-nil error. But when the context is cancelled in starttelemetrysession(), it results in a nil error. Another way to test would be to test for either of the condition to be true. Let me try that out and i can re-add both the conditions in a later PR.
Summary
Move the periodic timeout implemented by ACS and TACS to common websocket client library.
Implementation details
Testing
New tests cover the changes: Yes
Description for the changelog
Move the periodic timeout implemented by ACS and TACS to common websocket client library.
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.