-
Notifications
You must be signed in to change notification settings - Fork 25
123: allow managing auth data in memory only #126
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
Conversation
- supported init patterns shown in test_client.py
|
Hi @lindsay-stevens – thanks so much for the quick review of the issue and these changes!
Got it, thanks for sharing the rationale behind that design.
Yes! I also was hoping to write a separate issue for that one, since I felt they could be handled separately for ease of implementation and review. Nice that this PR tackles them both, though! I can update the issue accordingly, if that'd be useful. |
|
Thanks for filing a detailed issue and proposing a solution, @juliemacdonell! If I understand correctly, @lindsay-stevens' alternative here fully satisfies your needs, right? If so, let's go for this. The Even though it's not a preferred path, I do think we should have some documentation. My suggestion:
How does that sound to you two?
My feeling is that this PR explains the issue and solution well so it's not necessary! |
|
|
||
|
|
||
| def get_config_path(config_path: str | None = None) -> Path: | ||
| def get_config_path(config_path: str | Path | None = None) -> Path: |
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.
non-blocking: I think this function is not used anywhere. Perhaps it can be removed?
Yes! Thanks, @lognaturel and @lindsay-stevens. |
Closes #123
What has been done to verify that this works as intended?
Added tests in test_client with patches to check for API stability and skippable E2E tests that were run against staging.
Why is this the best possible solution? Were any other approaches considered?
The absence of auth parameters on the
Clientwas a design choice to discourage putting credentials directly into scripts (e.g. in a notebook file like in thedocs/examplesdirectories) and instead encourage using a home/private directory since one of the motivating use cases for pyodk is the analyst and/or new coder.However there have been requests from some more advanced library users that want to manage the auth data in memory and/or avoid writing files. Since pyodk writes the auth token to a cache file, a solution needs to account for that as well. So this PR implements a new pattern for passing in a Config object, which can optionally not cache the session to a file and thereby not touch any files at all. As noted in
test_client.pyit seems like Central caches sessions as well anyway.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Should be backwards compatible - I think the only clash would be if someone was importing
configfrompyodk.client, which is now aliased tocfgto avoid a name clash with the newconfigparameter. If that is a problem then the fix is just to replaceconfigwithcfgin the user's code import statement.Do we need any specific form for testing your changes? If so, please attach one.
Some of the tests require a Central instance but these are skipped for CI.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
As mentioned above the new functionality is intended for advanced use cases so maybe it's better to not document it in the
readme? It is at least demonstrated in the tests.Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyodk testsandruff check pyodk teststo lint code