feat: wrap generate_keys() in future
#168
Conversation
RahulDubey391
requested review from
cthumuluru,
nancynh and
a team
as code owners
RahulDubey391
changed the title
|
Hi @RahulDubey391! Thanks for the PR 😄 It still needs a few things to be fully complete. You will need to update your code to add an You will also want to The command to run the unit tests is |
I have added the await where ever the key generation is happening, but after running the unit tests, the code is failing. I am pushing the changes although, will keep on testing until the tests are passed.
|
@RahulDubey391 please take a look at failing tests and lint jobs. Check my previous comment for running these locally and making sure they work. If this is too much trouble I am happy to jump in and help finish off this PR for you 😄 Thanks again, we really appreciate the contribution |
|
Hi @jackwotherspoon, thanks for helping me out! After your suggestions, I re-ran the test cases. Lint Checks have passed already. For Unit Test cases, I am facing issue with 3 particular cases. 20/23 have passed already. I need your help in other 3 cases. These cases were:
Can you guide me around how can I fix the above? Also for the System Test cases, I am getting the "ALLOYDB_INSTANCE_IP" environment variable KeyError. |
|
@RahulDubey391 Looks like tests are all green actually 👍 🟢 I will give the PR a final look over on Monday morning, otherwise we should be good to go ahead and merge. Thanks again! |
Thanks for the help and guidance @jackwotherspoon, much appreciated! |
| @@ -53,7 +54,7 @@ def __init__( | |||
| self, | |||
| instance_uri: str, | |||
| client: AlloyDBClient, | |||
| keys: Tuple[rsa.RSAPrivateKey, str], | |||
| keys: asyncio.Future, | |||
There was a problem hiding this comment.
I wonder if we can try to type hint the return of the future? I believe this was introduced in Python 3.8 so we should be okay.
| keys: asyncio.Future, | |
| keys: asyncio.Future[Tuple[rsa.RSAPrivateKey, str]], |
There was a problem hiding this comment.
Hi @jackwotherspoon , thanks for the feedback! I have pushed the above change in the latest push
tests/unit/test_instance.py
Outdated
| event_loop = asyncio.get_running_loop() | ||
| keys = asyncio.wrap_future( | ||
| asyncio.run_coroutine_threadsafe(generate_keys(), event_loop), loop=event_loop | ||
| ) |
There was a problem hiding this comment.
I think we can just await this directly here...? The reason we have to use the future in the main code is because the loop is running in a separate thread. The tests run in the same thread so I think we can just await it directly.
| event_loop = asyncio.get_running_loop() | |
| keys = asyncio.wrap_future( | |
| asyncio.run_coroutine_threadsafe(generate_keys(), event_loop), loop=event_loop | |
| ) | |
| keys = await generate_keys() |
There was a problem hiding this comment.
same for other test cases
There was a problem hiding this comment.
Hi @jackwotherspoon , I have removed the separate Async loops.
But one doubt, since the keys are being passed to the Instance object as future, due to double await, the Tests are throwing error that "an awaited expression can't be awaited again". So I removed the await for the generate_keys() function so finally it's being awaited once in the main Instance loop.
Tell me if it's fine otherwise we can revert it back.
jackwotherspoon
changed the title
generate_keys() in future
Hi @jackwotherspoon , thanks a lot! Sure if you can take over the last issue with Async tests failing in some cases, that would be much helpful. But do let me know what works best, I am also pushing my changes. |
Done! Made the tests use Will go ahead and merge this, thanks again for the contribution! 👏 |
Added the following changes to the files:
Fixes #14