Open romanett opened 1 month ago
Attention: Patch coverage is 81.42857%
with 13 lines
in your changes missing coverage. Please review.
Project coverage is 54.66%. Comparing base (
cf2e788
) to head (b98517b
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
There are a few helper defined already, e.g. in CertificateIdentifier.ParseBlob
and CertificateIdentifier.CreateBlob
, just as an example for the API. There is also Utils.ParseCertificateChainBlob()
and Utils.ParseCertificateBlob()
, you could provide a generic function to create a blob. The CertificateIdentifier Parse/Create code is not used much afaik (maybe in a GDS app?) and can be marked obsolete and removed in a while.
Please check also the CertificateUpdate
methods in HttpsTransportListener
and TcpTransportListener
. The way the descriptions are updated also seems not quite right?
There are a few helper defined already, e.g. in
CertificateIdentifier.ParseBlob
andCertificateIdentifier.CreateBlob
, just as an example for the API. There is alsoUtils.ParseCertificateChainBlob()
andUtils.ParseCertificateBlob()
, you could provide a generic function to create a blob. The CertificateIdentifier Parse/Create code is not used much afaik (maybe in a GDS app?) and can be marked obsolete and removed in a while.
I could not find any usage of CertificateIdentified ParseBlob /CreateBlob. I will mark them obsolete and instead point to Utils. ParseCertificateChainBlob() / create Utils.CreateCertificateChainBlob
Please check also the
CertificateUpdate
methods inHttpsTransportListener
andTcpTransportListener
. The way the descriptions are updated also seems not quite right?
On startup the CertificateChain is Initialized always to the full chain, only on certificate update an empty CertificateChain is constructed. I will switch to this to ensure consistent behaviour of the property also after certificate update.
Why does Session have a Property ServerCertificateChain that is never actually initialized with the ServerCertificateChain, can it be removed?
Proposed changes
Update the Server Endpoint Descriptions after certificate update e.g. from a GDS Push. Until this PR this needed a resart.
Confirmed working with the Discovery client and this code:
Related Issues
Types of changes
Checklist
Further comments