auth: add dictionary storage #204
Conversation
efiop
changed the title
efiop
force-pushed
the
auth
branch
2 times, most recently
from
b127769 to
0938c78
Compare
efiop
force-pushed
the
auth
branch
10 times, most recently
from
f7068fa to
2fddaca
Compare
| storages, default = self._InitializeStoragesFromSettings() | ||
| self._storages = storages | ||
| self._default_storage = default |
There was a problem hiding this comment.
Original names are a bit unwieldy, so useing temp short ones. Not the prettiest, but I'm trying to make minimal changes.
efiop
force-pushed
the
auth
branch
2 times, most recently
from
1742f0c to
2495f54
Compare
efiop
changed the title
efiop
changed the title
efiop
changed the title
|
@shcheklein Ready for review. |
Allows not to save anything to disk, which is useful overall but especially useful when working with env vars, which could be sensitive so saving their value to disk is undesirable.
| @@ -347,11 +348,16 @@ def _InitializeStoragesFromSettings(self): | |||
| "Please specify credentials file to read" | |||
| ) | |||
| result[backend] = Storage(credentials_file) | |||
| elif backend == "dictionary": | |||
| result[backend] = DictionaryStorage( | |||
| self.settings["save_credentials_dict"], | |||
There was a problem hiding this comment.
In the file case there is a check if credentials_file is None .... raise InvalidConfigError should we do the same? (probably should be part of ValidateSettings in a single place).
There was a problem hiding this comment.
Ah, good point, missed that. Will adjust.
| :save_credentials_file (str): Destination of credentials file. **Required**: Yes, only if *save_credentials_backend* is 'file'. | ||
| :save_credentials_dict (dict): Dict to use for storing credentials. **Required**: Yes, only if *save_credentials_backend* is 'dictionary'. | ||
| :save_credentials_key (str): Key within the *save_credentials_dict* to store the credentials in. **Required**: Yes, only if *save_credentials_backend* is 'dictionary'. |
There was a problem hiding this comment.
feels redundant tbh ... use can do [save_credentials_key] on their own ... do you have some DVC case in mind?
There was a problem hiding this comment.
or is it a default convention in the DictionaryStorage?
There was a problem hiding this comment.
Yeah, it is default convention in DictionaryStorage. I went with a simpler solution initially, but this is indeed more flexible and more consistent.
There was a problem hiding this comment.
thanks @efiop ! a few comments, approved so that we don't block it
| ga.ServiceAuth() | ||
| assert not ga.access_token_expired | ||
| assert creds_dict == first_creds_dict | ||
| time.sleep(1) |
There was a problem hiding this comment.
I see that's it's a convention here ... no worries then, not clear why it's needed though :)
There was a problem hiding this comment.
It is testing that we reused the creds and didn't generate new ones. A bit vague but it does the job.
There was a problem hiding this comment.
sorry, I meant the time.sleep(1) :)
Allows not to save anything to disk, which is useful overall but especially
useful when working with env vars, which could be sensitive so saving their
value to disk is undesirable.