tests/functional: assert R_OK/W_OK for XDG dirs #16322
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
See cabotage/cabotage-app#85. Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
| user_data = platformdirs.user_data_dir(ensure_exists=True) | ||
| user_cache = platformdirs.user_cache_dir(ensure_exists=True) |
There was a problem hiding this comment.
NB: With ensure_exists this is now partially a no-op test, at least in local/CI testing (since these dirs don't exist). However for Cabotage/deployment it'll end up testing something useful, per cabotage/cabotage-app#85
There was a problem hiding this comment.
just for clarity, we do not execute tests in the cabotage environment. should this be a runtime check?
There was a problem hiding this comment.
Ah, didn't realize that -- yeah, in that case it probably makes more sense as a runtime check. I'll look at putting it somewhere early in python -m warehouse's initialization 🙂
There was a problem hiding this comment.
I've put this in warehouse/config.py, as part of the early initialization that happens there. But I'm not 100% sure if that's the best place 🙂
There was a problem hiding this comment.
In smoke testing this, it looks like this doesn't correctly detect that things are working.
I rolled out a new release of test.pypi.org using this wrapper and you can see results in cabotage/cabotage-app#85 (comment)
platformdirs.user_data_dir(ensure_exists=True) succeeds, but platformdirs.user_data_dir('app', ensure_exists=True) fails, indicating that this wouldn't actually ensure success for xdg/platformdirs use later on.
There was a problem hiding this comment.
appears this is due to the existing /tmp/{share, cache} files created by the Dockerfile on main :)
There was a problem hiding this comment.
validated this locally!
I still think it may be wise to change these to platformdirs.user_cache_dir(secrets.token_urlsafe(), ensure_exists=True) and similar to ensure not only that the dirs exist but that they're writable.
There was a problem hiding this comment.
appears this is due to the existing /tmp/{share, cache} files created by the Dockerfile on main :)
Whoops 🤦 -- I think merging this first should fix that then, since we won't be doing mkdir in the Dockerfile anymore.
There was a problem hiding this comment.
I still think it may be wise to change these to
platformdirs.user_cache_dir(secrets.token_urlsafe(), ensure_exists=True)and similar to ensure not only that the dirs exist but that they're writable.
Done!
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
|
Note: this successfully deployed, so appears to be working end-to-end with the changes from cabotage-app. |
Awesome. I'll attempt to kick the tires again 🙂 |
This follows #16304 -- this test passes for local and CI runs because they don't use the
nobodyuser, which then fails at runtime (in deployment) to access these dirs.cabotage/cabotage-app#85 fixes the above by moving the XDG dir creation to within Cabotage's wrapper; this updates the backstop test to be permission/access based, rather than existence based.