-
Notifications
You must be signed in to change notification settings - Fork 70
Adding encryption example using a KMS and JWT-based auth #138
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
base: main
Are you sure you want to change the base?
Conversation
Thanks! Added some review comments. Also may want to |
Thanks @cretz for all of the great feedback. I'll make these updates in the next day or two and will let you know! |
Thanks for all the suggestions, @cretz. I think I've addressed everything. Let me know if you have any more feedback. |
Make need to run |
Should be good to go now, @cretz. Thanks again! |
@phillipskevin - Looks like there's a type error. Specifically, since we allow our Python samples to run in in all non-EOL Python versions, you have to change things like For this error:
You may need to add a dependency for https://pypi.org/project/types-requests. Have not investigated this. |
Looks like CI is still failing after adding the |
Looks like it's still complaining on 3.8. I admittedly have not spent time digging into this. |
Python 3.8 went end of life last week, so having some trouble testing it now. Will the versions used in CI be updated? Or will you continue to support 3.8 for some time? |
I suspect we will EOL it too soon, though no specific timeline. EDIT: Opened temporalio/sdk-python#672 for dropping 3.8 support. |
Hey @cretz , Happy Friday! Here's some screenshots of my recent commits to this PR that successfully run the below commands locally - would you be willing to approve the workflows to run (when you get a chance)?
|
Approved to run CI now, thanks for sticking with this! Also, if possible, any kind of tests to confirm this remains working would be helpful (we have had some contributed samples become stale and stop working for one reason or another). |
logger = logging.getLogger(__name__) | ||
app.add_routes( | ||
[ | ||
web.post("/encode", make_handler("encode")), |
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.
Do we need to expose the encode over http?
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.
It's a good idea to. Our web UI and CLI use the encode side when you are starting workflows, sending queries, etc from those interfaces.
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.
If possible, any tests would help here, though I know it can be a bit difficult since the cloud JWKS is hardcoded. We can discuss merging without tests if they are too hard to write or they need to be done after merge. (we know many of our samples like the encryption sample don't have tests and we keep getting burned by not having them)
No description provided.