Add refresh function in panda_auth #92
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 31.90% 36.26% +4.35%
==========================================
Files 12 13 +1
Lines 1147 1249 +102
Branches 193 204 +11
==========================================
+ Hits 366 453 +87
- Misses 769 776 +7
- Partials 12 20 +8 ☔ View full report in Codecov by Sentry. |
zhaoyuyoung
force-pushed
the
tickets/DM-48912
branch
5 times, most recently
from
57b70b4 to
ed11c91
Compare
MichelleGower
left a comment
MichelleGower
left a comment
There was a problem hiding this comment.
Some suggestions. Also it would be good to have unit tests, but not sure how difficult mocking the various calls would be.
|
|
||
|
|
||
| def panda_auth_refresh(days=4, verbose=False): | ||
| """Refresh auth token""" |
There was a problem hiding this comment.
Need the parameters description in the docstring.
| print("=" * 60 + "\n") | ||
| return | ||
| else: | ||
| print("Cannot find token file.") |
There was a problem hiding this comment.
Perhaps point the user to the command should use instead?
| status = panda_auth_status() | ||
| if status: | ||
| print(f"{'New expiration time:':23} {datetime.utcfromtimestamp(status['exp'])} UTC") | ||
| print("Success to refresh token") |
There was a problem hiding this comment.
No failure status nor exception is being thrown in all the failure cases. This means the function nor command can be used in a script/program where want to halt on error. If some of these one might not want to hard fail (e.g., maybe the too early to refresh), could throw different exceptions and let the calling program decide.
There was a problem hiding this comment.
If do this, then many of the user messages can be moved to the driver upon catching the exception instead of in this function.
There was a problem hiding this comment.
I added a couple exceptions and changed those printing to raise. I also added simple unit tests for the refresh function.
| print(f"Token will expire in {minutes} minutes.") | ||
| print(f"Token expiration time : {exp_time.strftime('%Y-%m-%d %H:%M:%S')} UTC") | ||
| if delta < timedelta(minutes=0): | ||
| print("Token already expired. Cannot refresh.") |
There was a problem hiding this comment.
Here you need to check the exp time of the refresh token, not the id_token. For cases, the id_token lifetime can be 24 hours, but the refresh_token can be 30 days or even longer.
There was a problem hiding this comment.
oh. It's correct. If the token is expired (even the refresh token is valid), you should not be able to connect the server to refresh the token. So this part is correct.
zhaoyuyoung
force-pushed
the
tickets/DM-48912
branch
3 times, most recently
from
4dd24d2 to
73d0882
Compare
zhaoyuyoung
force-pushed
the
tickets/DM-48912
branch
from
73d0882 to
8e767e3
Compare
Checklist
doc/changes