get login from env every time #63
Conversation
|
I worry that some users will not be sophisticated enough to set environment variables (e.g. windows power shell) we should check in with @geoffhancock to gauge this. |
| self.password = password or os.getenv("WATTTIME_PASSWORD") | ||
|
|
||
| if username: | ||
| os.environ["WATTTIME_USER"] = username |
There was a problem hiding this comment.
the intent of this was to allow users to pass in these parameters, but not save them as attributes that could be pickled.
setting an env var using os.environ is only persistent for the current session.
There was a problem hiding this comment.
This PR's is intended to preserve the signature, i.e. users can still pass these parameters. But the class handles them differently: it stores them in os.environ, then retrieves them each time they are needed.
There was a problem hiding this comment.
oh I misunderstood the history here. This change is already on future-release, and is being picked over to main?
There was a problem hiding this comment.
No this is a new approach. The change on future-release stored the password as a class property whereas this stores it in the sessions' environment.
There was a problem hiding this comment.
setting an env var using os.environ is only persistent for the current session.
It's good to check that we can re-login, but I think we are robust to this still. I will attempt to write a test to verify re-login on an expired token works as expected.
There was a problem hiding this comment.
the approach on future-release does not store anything in the class property as far as I can tell. I believe it's the same approach as this with additional logging.
watttime-python-client/watttime/api.py
Line 70 in 2274bc3
I think that logging can be helpful, but if you'd rather move ahead with out it thats ok by me.
I would guess it is a mix, and even if a majority know how, it is unlikely that everyone knows how to do it. It is actually quite easy to do in Windows, but not everyone has necessarily been exposed to it. Requiring it might be asking too much, but I like the approach of allowing both ways and nudging toward using envars. |
The intent here is to not change behavior at all but to reduce the surface area over which these parameters exist. If it cannot be done with this approach my next approach will be to try the system keyring. As a last resort, I would suggest we revisit this discussion and explore changing this functionality. |
What
Read login credential from os.environ every time
Why
This will reduce the surface area of credentials to only the process's os.evniron
How
Read directly from os.enrion every time a credential is necessary, e.g. the first login or to refresh a token.