-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat(common.opcua): Add session timeout as configuration option #15341
feat(common.opcua): Add session timeout as configuration option #15341
Conversation
Thanks so much for the pull request! |
!signed-cla
|
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.
Thanks @maxime-jolliet-airseas for your contribution! I do have some comments in the code. It would also be nice to see a unit-test for the new option if possible...
Hi srebhan, thank you for taking care of this PR. |
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.
@maxime-jolliet-airseas thanks for the quick update!
Please do not change existing tests (see comments in the code) as we are using those to check backward compatibility of changes... I also do have a suggestion to only convert the timeout if set.
Regarding tests, the only way I see is to set the session timeout to a very low value (e.g. 100 millisecond), establish a session and then check if it actually timed out... But not a must...
Ok, I removed the tests modifications. |
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.
Awesome! Thanks @maxime-jolliet-airseas!
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.
Thanks for the PR! I am going to remove the "default is..." statement as the value in the config is the default.
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
Summary
Adding the parameter "session_timeout" to OPCUA plugins configuration.
Checklist
Related issues
resolves #15258