-
Notifications
You must be signed in to change notification settings - Fork 413
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
[opcua] Implementation of client side security policy #1007
Conversation
Hi, do you need any help with your PR? Would also be awesome, if you could sign up to our mailinglist and get a bit more in touch with the rest of the community [email protected] (subscribe by sending an empty email to [email protected]). Chris |
2f89cb4
to
5b5caf1
Compare
@takraj Can you reserve some time next week to test this PR with your environment? Subscription tests are currently failing due to incomplete logic update, but these changes should address several aspects, including threading within driver. |
624162b
to
4defc9d
Compare
9ce4238
to
0b1bc6b
Compare
Hey folks, could you have a look on these changes? I am working on last fixes for windows unit tests (I'll probably remove |
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.
Had a look at mainly the changes in the shared parts (SPI) no objections.
Didn't review the OPC-UA stuff, as you know I usually don't care about OPC-UA ;-)
But thanks for your work on it :-)
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/generation/WriteBufferByteBased.java
Outdated
Show resolved
Hide resolved
f327ff9
to
5a1834f
Compare
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.
Nothing holding you out from merging this.
Had a quick look, the existing functionality still seems to be working.
I haven't tested the security stuff yet, but will do eventually (Although a short readme on how to generate a keystore wouldn't go astray ;) )
@splatch I would recommend the PR then ASAP otherwise we get into merge issues again at some point :) |
Fair enough, I'll first rebase and then get a fresh sign off from @PatrykGala. For some reason github looses commit author when rebase is being made. |
Well ... you could merge in changes? Doesn't need to be a rebase ... especially when we do a squash merge back, right? |
Rebase is not an issue. I think hardest part was getting it after all fixes we had prior 0.11 release. I'd prefer to keep these four commits as they are, simply because first one is made by someone else (maybe we can squash failing unit test). Then there is mspec update which, for ease of porting to other languages, I'd leave alone too. Last commit is update of driver logic and adjustment of encryption logic which is large enough on its own to be left alone. |
The one thing I'm a bit worried about is that the mspec changes seem to imply a completely different structure ... so I'm a bit worried about which ones are actually the right ones? Were the changes only affecting message types that haven't been used in the past (Possibly still not being used)? If they were not used in the past, I guess updating the mspec doesn't cause any harm. If they have been used, I wonder which is the right format. One option would be to create a new "version" of the mspec (You know that we can provide multiple versions of an mspec and simply reference the version we want in the code-generator. So technically we could reference version 2 in java and version 1 in go. |
Had a look at the stuff outside of the OPC-UA driver directories and have no objections to merging this. |
@chrisdutz I saw the dataio updates and I strive to merge this before you do. ;-) I will rebase it on top of current develop and wait for Patryk to amend sign-off on his commit. Today we will merge that, despite of remaining stability issue mentioned in #1364. |
Even if you would get a conflict with the generated code, it should be now problem as it always can be solved by regenerating :) |
5a1834f
to
53c3553
Compare
ae7222c
to
d70e355
Compare
5364094
to
362a99a
Compare
e1d7d0c
to
0c22ae7
Compare
0c22ae7
to
6b033d3
Compare
no text TBD
fixes #625
fixes #642
fixes #621
fixes #595