-
Notifications
You must be signed in to change notification settings - Fork 4
feat(webdav): add bearer_token_command for dynamic token acquisition #95
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
feat(webdav): add bearer_token_command for dynamic token acquisition #95
Conversation
771e527 to
d8c3969
Compare
d8c3969 to
da92966
Compare
21f7c04 to
17c381f
Compare
skshetry
left a comment
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.
Thank you for contributing. This looks good overall. One question. Could you also add a few tests to exercise the BearerAuth code path? That would help ensure it works correctly and does not regress.
Also, contribution to dvc repositories now require accepting the CLA: https://cla-assistant.io/treeverse/dvc-webdav. It'd be great if you could do that. Thanks.
|
@skshetry |
dvc_webdav/tests/test_bearer_auth.py
Outdated
| # indicating no further retries will be attempted. | ||
| with pytest.raises(StopIteration): | ||
| flow.send(resp_401_again) | ||
|
|
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.
I don’t see the need for tests beyond this line, especially the concurrent one. I doubt they would help catch the issue if it occurs in the future.
When I requested tests, my only concern was verifying that the command is called and behaves as expected: setting the token in the header on success and raising an error when it exits with a failure. Another goal was to ensure it works on Windows, though the current test doesn’t cover that since all subprocess calls are mocked and I’m okay with that.
I appreciate that you also added tests for token refresh, but the rest feels excessive.
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.
Thanks for the feedback!
I agree that the test suite was getting a bit heavy. I've removed test_auth_flow_failure_after_refresh and the other edge-case tests as suggested.
It looks much cleaner now. Please let me know if there's anything else needed.
|
@skshetry |
|
@GreenHatHG, looks like we don't have |
Fixed in the latest commit. |
|
Thank you for contributing! |

see: treeverse/dvc#10917