Check for common permissions issues with scratch area #765
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #765 +/- ##
==========================================
+ Coverage 92.92% 93.32% +0.40%
==========================================
Files 37 38 +1
Lines 2063 2083 +20
==========================================
+ Hits 1917 1944 +27
+ Misses 146 139 -7 ☔ View full report in Codecov by Sentry. |
| directory_path_no_permissions: Path, | ||
| ): | ||
| config = ScratchConfig(root=directory_path_no_permissions, repositories=[]) | ||
| with pytest.raises(PermissionError): |
There was a problem hiding this comment.
TODO: Match raisesses
callumforrester
force-pushed
the
747-file-lock
branch
from
74e879b to
80b0a42
Compare
callumforrester
changed the title
src/blueapi/cli/scratch.py
Outdated
| def _validate_root_directory(root_path: Path, required_gid: int | None) -> None: | ||
| _validate_directory(root_path) | ||
|
|
||
| if not is_sgid_enabled(root_path): |
There was a problem hiding this comment.
minor: I think it's more common/natural to talk about sgid being 'set' rather than enabled.
| bool: True if the SGID bit is enabled | ||
| """ | ||
|
|
||
| mask = os.stat(path).st_mode |
There was a problem hiding this comment.
minor: if we're expecting a Path here we could use the stat() method directly
| mask = os.stat(path).st_mode | |
| mask = path.stat().st_mode |
There was a problem hiding this comment.
Nice, I didn't know about that
tests/unit_tests/cli/test_scratch.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def directory_path() -> Generator[Path]: | ||
| def directory_path_no_permissions() -> Generator[Path]: |
There was a problem hiding this comment.
directory_path_no_sgid? The directory does have some permissions. Could also make sense to keep the directory_path fixture as it was and have a new sgid_directory_path fixture with the added permission set.
There was a problem hiding this comment.
How about directory_path_unchanged_permissions? I'm expecting that as we catch more issues over time we will need more tests that deal with a variety of different permissions setups
There was a problem hiding this comment.
_unchanged_permissions would be better but in that case I'd err on the side of leaving directory_path as the default and adding the changed versions as directory_path_with_sgid etc.
Or keep them all explicit (if a bit verbose) with directory_path_2766, directory_path_0666 etc
(I am aware this is getting pretty picky though - definitely not something that would block the PR)
| @@ -74,9 +76,6 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No | |||
|
|
|||
| _validate_directory(path) | |||
|
|
|||
| # Set umask to DLS standard | |||
| os.umask(stat.S_IWOTH) | |||
There was a problem hiding this comment.
Should this be removed from ensure_repo as well?
There was a problem hiding this comment.
Hmm, there's another test which clones to a tempdir that only passes because of this line (because the test calls ensure_repo directly rather than invoking the CLI. Maybe the test should use mocks instead, although I do worry that sticking the umask set in the CLI isn't as robust as I thought. Might be better to pepper the code with some ensure_correct_umask function in strategic places...
There was a problem hiding this comment.
...I now think we'll have to mock out all the directories in these tests anyway because of a separate issue that is breaking the CI with test_setup_scratch_succeeds_on_required_gid. I'll make an issue for that.
callumforrester
force-pushed
the
747-file-lock
branch
from
6bff7f0 to
928c7b0
Compare
Fixes #747