temporalio / sdk-core

Core Temporal SDK that can be used as a base for language specific Temporal SDKs
MIT License
262 stars 70 forks source link

client: clarify encoding of ClientTlsConfig field data #683

Closed djc closed 6 months ago

djc commented 6 months ago

What was changed

Add a _pem suffix to both ClientTlsConfig fields, and remove the client_ prefix (which is redundant with the ClientTlsConfig type name) to try to balance the length increase.

Why?

While trying to get a Rust SDK worker connected to Temporal Cloud, I got a TonicTransportError(tonic::transport::Error(Transport, PrivateKeyParseError)). This turned out to because I was passing DER-encoded private key and certificate to ClientTlsConfig instead of PEM. tonic's Identity::from_pem() makes it obvious that they should be PEM, but that context isn't obvious when looking at the rustdoc for temporal_client::ClientTlsConfig (and in fact, because it uses a Vec<u8> type instead of a String type, it suggests using DER over PEM).

Checklist

  1. How was this tested: no logic affected, so relying on the compiler
  2. Any doc updates needed: should be reflected in rustdoc automatically
djc commented 6 months ago

I've rolled back the field rename, and added a commit that cleans up a clippy issue likely new in Rust 1.76.

Sushisource commented 6 months ago

@djc This and your other PR need a rebase/merge. Normally I can do that from right in here but the option isn't showing up. I'm guessing maybe because you haven't allowed commits from maintainers: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

djc commented 6 months ago

@djc This and your other PR need a rebase/merge. Normally I can do that from right in here but the option isn't showing up. I'm guessing maybe because you haven't allowed commits from maintainers: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Hmm, I'm not seeing this checkbox show up.

Is there any chance/method by which you can put me on an allowlist to allow CI to run without waiting for explicit approval? What with the timezone differences, these feedback cycles are pretty slow. :(

Sushisource commented 6 months ago

@djc This and your other PR need a rebase/merge. Normally I can do that from right in here but the option isn't showing up. I'm guessing maybe because you haven't allowed commits from maintainers: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Hmm, I'm not seeing this checkbox show up.

Is there any chance/method by which you can put me on an allowlist to allow CI to run without waiting for explicit approval? What with the timezone differences, these feedback cycles are pretty slow. :(

I've changed it to only requiring approval for new contributors.

djc commented 6 months ago

I've changed it to only requiring approval for new contributors.

Thanks! Rebased this on top of current master, I guess that will do the job?

djc commented 6 months ago

The remaining test failure looks unrelated to this change?

Sushisource commented 6 months ago

The remaining test failure looks unrelated to this change?

Yeah it flakes occasionally. I'll re-run it and merge it.