sslcontext-kickstart icon indicating copy to clipboard operation
sslcontext-kickstart copied to clipboard

Published 20 hours ago • verified publisher icon Hakky54

Extra checks for single trust managers

Open ivenhov opened this issue 1 year ago • 1 comments

This is pull request for https://github.com/Hakky54/sslcontext-kickstart/issues/196

I had to do a change which allowed the project to compile in Eclipse For some reason Eclipse was complaining and could not resolve the type in one of the methods in KeyManagerUtils.

Changes in TrustManagerUtils break some tests. Some of them are simply extra assertions on logging messages which likely should be removed since for single trust managers there is no wrapping in CompositeX509ExtendedTrustManager Some failures are more likely related to the fact the tests are expecting it is possible to set up truststore with a trust manager that has no accepted issuers. I haven't looked into this

New changes deserve extra tests but it depends if other tests should be change first Thanks Daniel

ivenhov avatar Aug 04 '22 00:08 ivenhov

I am able to provide more detailed feedback from Tuesday onwards.. I currently don't have access to my development machine till that date. Thank you for the initial changes!

Hakky54 avatar Aug 04 '22 08:08 Hakky54

Hi @ivenhov Thank you for this pull request! I have added couple of small remarks :)

Hakky54 avatar Aug 09 '22 06:08 Hakky54

It seems like my remarks where not visible for you, I just submitted it again 😄

Hakky54 avatar Aug 11 '22 22:08 Hakky54

@ivenhov how is it going on your side, do you need any help?

Hakky54 avatar Aug 17 '22 07:08 Hakky54

I resolved some of the conversations I agreed with I haven't check in the code yet however as others needs some clarifications Is that the way to do it? Do you want me to squash the commits of the changed with already existing ones once all is sorted? Or should I commit on top of existing ones? What would be the best practice here? Sorry I don't usually work with Github

Thanks D.

ivenhov avatar Aug 18 '22 23:08 ivenhov

I resolved some of the conversations I agreed with I haven't check in the code yet however as others needs some clarifications. Is that the way to do it?

That is great, looking forward to it. Just commit and push your changes so I can have a look at it.

Do you want me to squash the commits of the changed with already existing ones once all is sorted? Or should I commit on top of existing ones? What would be the best practice here? Sorry I don't usually work with Github

I usually squash the commits before merging. However GitHub has this option built in so you don't need to do it on your side. It is up to you whether you want to squash it first on your machine before pushing.

Hakky54 avatar Aug 19 '22 07:08 Hakky54

@ivenhov how is it going on your side, do you need any help? No worries if you don't have time for this, I can also make the changes.

Hakky54 avatar Aug 26 '22 09:08 Hakky54

Hi @ivenhov I could not get any response from you so I decided to fix this on my side as I have planned to make a release soon to provide the fix for the end-users. I appreciate your effort on the initial PR 😊

Hakky54 avatar Aug 30 '22 09:08 Hakky54