search

opendatahub-io / caikit-nlp-client

A grpc and http client library for caikit-nlp
Apache License 2.0
3 stars 6 forks source link

feat(clients): :sparkles: Make GrpcClient accepting filepaths instead of certificate/keys bytes #79

Closed z103cb closed 11 months ago

z103cb commented 11 months ago

Updated the grpc client to accept either a path (string) to the cerificate file or a byte array with the cerificate contents

Closes #74

codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (6f3d4c1) 92.83% compared to head (50a5507) 93.08%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #79 +/- ## ========================================== + Coverage 92.83% 93.08% +0.25% ========================================== Files 13 13 Lines 684 709 +25 Branches 142 151 +9 ========================================== + Hits 635 660 +25 Misses 33 33 Partials 16 16 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bdattoma commented 11 months ago

@z103cb I think it would be better to have same behavior btw gRPC and HTTP clients. Is http client already supporting the feature implemented in this PR?

z103cb commented 11 months ago

The http client only accepts strings (paths) as arguments. Taking bytes as an argument, is a bit problematic, as it would force the the library (client) to manage the certificate life cycle (create, delete, remove). I am not convicent that we should do it. If simetry between clients is a harder requirement, at most I am willing to do is drop the support for bytes as an argument type.

@dtrifiro what is your take ?

dtrifiro commented 11 months ago

@bdattoma the http client takes paths so far. The grpc client accepted bytes because that's the format that the underlying grpc library expects (contrary to requests accepting paths only). I'd keep both options for now in the grpc client, but ideally I think that using paths only would make the client usage easier.

bdattoma commented 11 months ago

@bdattoma the http client takes paths so far. The grpc client accepted bytes because that's the format that the underlying grpc library expects (contrary to requests accepting paths only). I'd keep both options for now in the grpc client, but ideally I think that using paths only would make the client usage easier.

Oh okay, then I think it makes sense to keep both the alternative for grpcurl and document it in the README. Thanks!