-
Notifications
You must be signed in to change notification settings - Fork 44
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
Adding capability to support PKI #509
base: master
Are you sure you want to change the base?
Conversation
// TODO Auto-generated catch block | ||
e.printStackTrace(); |
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.
Please use ILog.log().error(e.getMessage(), e)
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.
@mickaelistria I noticed that most of the other try catch blocks just throw and dont print anything out. There are no other ILog messages in that bundle. Maybe better to stick with the picture show and just throw?
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.
@mickaelistria Looking again, the software is supposed to be transparent and not required, so perhaps btter to not do anything in the catch block. Software does not even need to know an exception may have occurred.
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.
Please 🙏 try to keep a PR to a single commit. Amend and force push further changes. Then you won’t be asked to squash the commits later.
Test Results 375 files +366 375 suites +366 41m 4s ⏱️ + 8m 42s Results for commit 2c2b5bc. ± Comparison against base commit c9f0582. This pull request removes 2165 and adds 1861 tests. Note that renamed tests count towards both.
This pull request removes 4 skipped tests and adds 3 skipped tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
@mickaelistria Please recheck.. |
I must confess I can't connect that statement to the actual code, the code seem to enable |
@laeubi When there is no PKI enabled the default SSLContext is not TLS. |
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.
Please rebase, squash commits and add documentation somewhere why / what this change does, forcing JRE http does not look "right" just because TLS is enabled (?)
Check if PKI has been installed and configured
Update with option to install PKI from marketplace, org.eclipse.core.pki