improve testing for readability/writability of shared directories #4079
Conversation
motivaiton: cleaner code changes: user new FS APIs to test for readability/writability instead of attempting to create a temp file
tomerd
force-pushed
the
refactor/user-readability-writability-apis
branch
from
53a4d7d to
cea3856
Compare
|
@yim-lee wdyt? |
|
@swift-ci please smoke test |
tomerd
changed the title
| try withTemporaryFile(dir: path.parentDirectory) { _ in } | ||
| } catch { | ||
| self.observabilityScope.emit(warning: "\(path.parentDirectory) is not accessible or not writable, not using .netrc file in it: \(error)") | ||
| guard localFileSystem.exists(path) && localFileSystem.isReadable(path) else { |
There was a problem hiding this comment.
We need isWritable because NetrcAuthorizationProvider supports edits (addOrUpdate), or we remove that capability.
There was a problem hiding this comment.
addOrUpdate is outside the AuthorizationProvider protocol iirc, so theoretically you would need to cast into NetrcAuthorizationProvider to use it?
There was a problem hiding this comment.
Yes, addOrUpdate is not part of the AuthorizationProvider protocol currently, but I see potential of it being added later (e.g., for registry authn). With this in mind, we should make this isWritable because we are bound forget to change it later.
There was a problem hiding this comment.
@yim-lee hesitant about requiring write access to this one specifically since often its a readonly file
we should def consider that if we are to offer a "write" netrc utility. atm i dont think we do, we can also comment out that code with a comment to make it more clear?
There was a problem hiding this comment.
we should def consider that if we are to offer a "write" netrc utility
We would have to, for saving registry credentials.
There was a problem hiding this comment.
One option might be to modify NetrcAuthorizationProvider.addOrUpdate so it does the isWritable check each time, and then we keep this isReadable.
Or, since write would fail anyway, keep NetrcAuthorizationProvider.addOrUpdate as-is and let it throw the file system error.
There was a problem hiding this comment.
@yim-lee happy to add such check if you think its useful. lmk!
tomerd
changed the title
motivaiton: cleaner code
changes: user new FS APIs to test for readability/writability instead of attempting to create a temp file