Skip to content
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: lock file while read, write and refresh token #299

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

natthan-pigoux
Copy link
Contributor

@natthan-pigoux natthan-pigoux commented Sep 17, 2024

closes #133

Tests can be performed on read/write_credentials, and the client (probably simpler if we do it through get_token directly). Test cases:

  • (read/write_credentials and get_token) multiple processes attempting to read or write to the credentials file simultaneously.
  • (read/write_credentials) credentials file is missing, empty, invalid.
  • (get_token) token exists and is valid, credentials file is missing, empty, invalid, token needs to be refreshed, refresh token valid/invalid.

@aldbr aldbr marked this pull request as draft September 27, 2024 09:54
@chaen chaen force-pushed the feat/lock_refresh_token branch from 7315d77 to e259927 Compare October 16, 2024 10:48
@natthan-pigoux natthan-pigoux force-pushed the feat/lock_refresh_token branch from 276db27 to 523cf68 Compare November 5, 2024 15:12
@natthan-pigoux natthan-pigoux marked this pull request as ready for review November 5, 2024 15:18
@aldbr aldbr force-pushed the feat/lock_refresh_token branch 3 times, most recently from a36862b to bb29473 Compare November 20, 2024 17:07
@aldbr aldbr force-pushed the feat/lock_refresh_token branch 2 times, most recently from 9317732 to 5339cb4 Compare November 26, 2024 17:17
@aldbr
Copy link
Contributor

aldbr commented Nov 26, 2024

@natthan-pigoux

  • I fixed the mypy issues
  • I split the test in 2 different files to avoid importing diracx-client within diracx-core
  • I simplified a few parts in the tests (by using the tmp_path fixture for instance)
  • The logic to test file locking looked robust in terms of evolutivity but sounds complex to debug and flaky (e.g. use of time.sleep). I decided to mock flock instead, it's less robust (we would need change the tests if we use something else), but it looks like more deterministic, quicker, and easier to understand. If you are against that change, we can discuss.

@chaen chaen force-pushed the feat/lock_refresh_token branch from 5339cb4 to 8e5fea6 Compare December 6, 2024 08:58
@chaen chaen merged commit ad49f49 into DIRACGrid:main Dec 6, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use file locking to prevent multiple processes using the refresh token at the same time
3 participants