Fixed bug in SSL Seldon Client and added functionality in GRPC #946
Conversation
| root_certificates=open(channel_credentials.root_certificates_file, 'rb').read()) | ||
| # This piece also allows for blank SSL Channel credentials in case this is required | ||
| else: | ||
| grpc_channel_credentials = grpc.ssl_channel_credentials() |
There was a problem hiding this comment.
Would it make sense to build the dict of arguments as args = {"root_certificates: ..., "private_key": ..., ...} and then just pass it down as grpc.ssl_channel_credentials(**args) instead of the three if-else cases? This is mostly a personal preference though, so please ignore it if you don't agree.
On the other hand, and this is something that was already present on the codebase, does an inline call to open().read() also close the file after it's finished?
There was a problem hiding this comment.
The main reason was due to the open statement that is embedded (And yeah it was already in that format). I do agree, I think we'll have to review the whole GRPC / REST SSL at some point soon so not sure how much worth it would be to have it here.
In regards ot the second question, open files get closed and garbage collected when they lose the last reference pointer, so in this case they would be getting closed either immediately or when the function returns.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cliveseldon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What is being added: